Skip to content

fix(omp): make effect driving durable and deterministic - #1580

Open
panosAthDBX wants to merge 3 commits into
a5c-ai:mainfrom
panosAthDBX:fix/omp-deterministic-driver-recovery
Open

fix(omp): make effect driving durable and deterministic#1580
panosAthDBX wants to merge 3 commits into
a5c-ai:mainfrom
panosAthDBX:fix/omp-deterministic-driver-recovery

Conversation

@panosAthDBX

@panosAthDBX panosAthDBX commented Jul 23, 2026

Copy link
Copy Markdown

Summary

  • add the OMP deterministic effect driver with bounded shell execution and durable checkpoint recovery
  • preserve exact per-effect model routing and structured agent prompts
  • enforce retained single-writer agent ownership, immutable results, and automatic continuation
  • ship the driver and blocking bridge agent in generated OMP packages

Superseded by the combined upstream integration PR #1582, which contains this driver commit and the integration observability commit on top of it.

Related stack:

Verification

  • OMP deterministic driver regressions: 11/11 passed
  • OMP bundle regressions: 5/5 passed
  • OMP driver TypeScript check passed
  • extensions adapter build passed
  • metadata verification passed
  • diff hygiene check passed

Fixes #1578
Fixes #1579

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes based on the adversarial review process.

Blockers

  1. Skill effects are dispatched but cannot be re-entered after the first checkpoint

drive() routes both action.kind === "agent" and action.kind === "skill" through prepareAgentEffect() at plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:187. But prepareAgentEffect() persists the checkpoint as kind: "agent" at plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:426-431. On re-entry, validateCheckpointIdentity() compares that checkpoint kind with the current action kind at plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:910-912, so a pending/completed skill effect becomes an identity mismatch. That defeats the durable recovery path for a normal Babysitter effect kind the driver explicitly accepts.

Fix: either do not handle skill effects here, or persist the original effect kind separately from the bridge execution kind and add regressions for skill re-entry before claim, while owned, and after completed output exists.

  1. The durable owner completion channel can accept values that are not actually valid for strict schemas

completeAgentOwnerValue() relies on validateJsonSchema() before persisting/posting the owner value (plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:289, 318), but the validator only implements a small subset of JSON Schema (plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:801-833). It ignores additionalProperties, combinators, const, string/numeric constraints, and schemas with required/properties but no explicit type: "object". Since this direct owner channel is supposed to preserve one schema-valid immutable effect result, accepting schema-invalid values can still corrupt effect evidence.

Fix: use the host/tooling schema enforcement or include a real JSON Schema validator in the generated package, then add tests for additionalProperties: false and combinator/constraint schemas.

Major

  • Regression coverage misses the accepted skill-effect path. The new tests cover shell and agent paths, but no action("skill") case despite the production loop accepting skill effects. Please add focused coverage for skill dispatch/re-drive behavior.

QA

I dispatched qa-dispatch.yml for PR #1580. Run 30867341253 failed before product QA executed: actions/checkout@v6 could not find ref fix/omp-deterministic-driver-recovery in a5c-ai/babysitter. So QA has not passed for this PR.

Risk Assessment

Risk level: risk:high

  • Skill-based Babysitter runs can stop during OMP recovery/re-entry with unresolved requested effects. Mitigation: add the skill-effect regression and canary a shell + skill + agent process before release.
  • Schema-invalid owner results can be persisted as authoritative effect evidence. Mitigation: replace subset validation with complete validation and audit output/result agreement after staging.
  • This PR says it is superseded by #1582, which is also unstable. Mitigation: land one integrated stack after reconciling #1582 instead of merging this partial PR independently.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking this as-is.

This PR adds the right general shape for an OMP deterministic driver, but it breaks the normal Babysitter shell-task contract and the PR body also says this branch is superseded by #1582.

Findings

Blocker: shell output is read before the driver writes it

plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:853

shellResultValue() treats every successful shell task with io.outputJsonPath as if the command already created that file, and calls fs.readFile(outputPath) before resolveShellEffect() writes the driver-owned output.json. Existing process-library shell gates commonly declare io.outputJsonPath/shell.outputPath while the harness/driver is responsible for capturing the result artifact. For example, library/processes/shared/ts-check.js points outputJsonPath at tasks/<effectId>/output.json, but the tsc command does not create JSON there.

Under this PR, those ordinary shell effects fail with ENOENT instead of being checkpointed and posted, which defeats the core deterministic-driver purpose.

Fix: only read a command-owned output file when that contract is explicit and the file exists. For ordinary shell tasks, capture stdout/failure metadata and write the driver-owned output.json before task:post. Add a regression with shell.command, shell.outputPath, and io.outputJsonPath where the command exits 0 and writes only stdout.

Major: tests miss the real shell-task shape

packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:71

The new shell tests construct actions without io.outputJsonPath/shell.outputPath, so the read-before-write bug above is not exercised. Please add coverage for the process-library shell contract used by ts-check, fork compatibility, and visual smoke gates.

Major: the OMP entrypoint wiring is excluded from the advertised typecheck

plugins/babysitter-unified/per-harness/omp/tsconfig.json:7

The OMP tsconfig includes only extensions-driver.ts, but the integration wiring lives in extensions-index.ts via pi.registerTool() and pi.on("tool_call"/"tool_result"). API-shape issues in the actual OMP extension host path can slip past the claimed TypeScript check.

Fix: include extensions-index.ts in this tsconfig or add a separate OMP integration typecheck that covers the registered tools and event handlers.

QA

I dispatched qa-dispatch.yml for this PR. The returned run 30867348872 failed before scenarios ran: actions/checkout@v6 could not find fix/omp-deterministic-driver-recovery in a5c-ai/babysitter. That makes product QA inconclusive, but the requested QA gate did not pass.

Risk Assessment

Risk level: risk:high.

Risks and mitigations:

  • Normal shell effects can fail before task:post because outputJsonPath is read before durable output is written. Mitigation: fix output handling and run an OMP-driver regression against existing shell-gate task shapes.
  • OMP entrypoint wiring can drift from the host API because extensions-index.ts is not typechecked by the new OMP tsconfig. Mitigation: typecheck the integration entrypoint and run a generated-package OMP smoke test.
  • The PR says it is superseded by #1582. Mitigation: land the corrected implementation in the active combined PR instead of merging this stale standalone branch.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review decision

Requesting changes. This PR should not merge standalone. The PR body says it is superseded by #1582, and the standalone change still leaves critical integration safety gaps that #1582 explicitly addresses.

Blocker

  • plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:703 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:213: mutated bridge task envelopes can still claim durable ownership. parseAgentBridgeInput accepts any single babysitter-task item whose first line contains a syntactically valid BABYSITTER_OMP_BRIDGE descriptor, and claimAgentToolCall only compares the requested model before writing the owner file. It does not verify that the complete task envelope still matches the driver-generated prompt, name, output schema, or other immutable dispatch fields. A malformed or altered task payload with the original descriptor can therefore become the retained single writer and later commit a durable result for instructions the driver did not dispatch. Persist a canonical hash or full immutable dispatch envelope in the checkpoint and reject tool_call/tool_result/owner-completion paths unless the full envelope matches. Add regression coverage for altered prompt text, altered task name/agent fields, altered schema, and descriptor replay/mutation.

Major

  • packages/adapters/extensions/src/targets/adapters/oh-my-pi.ts:89: generated OMP packages still declare @oh-my-pi/pi-coding-agent: "*" even though this PR adds runtime use of newer OMP APIs in plugins/babysitter-unified/per-harness/omp/extensions-index.ts (pi.registerTool, pi.on("tool_call"), pi.on("tool_result"), pi.zod, and pi.exec result handling). Users on older or incompatible OMP releases can install a package that fails at activation/runtime. Constrain the OMP peer range to the minimum API version that provides these contracts and include a strict generated-package install/import compatibility test.

QA

  • Local targeted verification in an isolated worktree passed after installing/building prerequisites: npm run build:sdk, npx tsc -p plugins/babysitter-unified/per-harness/omp/tsconfig.json --noEmit, npm exec -- vitest run --config packages/adapters/extensions/vitest.config.ts packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts (11/11), npm run build --workspace=@a5c-ai/extensions-adapter, and npm run verify:metadata.
  • The requested qa-dispatch.yml run failed before scenarios executed: run 30867452453 failed in actions/checkout@v6 because it tried to checkout a5c-ai/babysitter ref fix/omp-deterministic-driver-recovery, but the PR head branch is on the fork panosAthDBX/babysitter. Treating QA as failed/inconclusive for this process.

Risk Assessment

Risk level: risk:high

  • Risk: a mutated or replayed bridge envelope can become the durable owner and write a successful result for the wrong assignment. Mitigation: add immutable envelope validation and mutation/replay regression tests; ship via the superseding integration PR that includes fail-closed envelope handling.
  • Risk: generated OMP package may install on unsupported OMP versions and fail at runtime. Mitigation: constrain the peer range and run strict generated package install/import compatibility tests.
  • Risk: merging this superseded PR alone omits integration hardening and observability already called out in #1582. Mitigation: close or leave #1580 superseded and review #1582 as the integration vehicle.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for this PR as currently scoped.

The focused local checks passed, but the PR is not approval-ready because the required QA dispatch failed before tests ran, the PR body says this change is superseded by #1582, and the driver has two high-risk integration gaps.

Findings

  1. Major: deterministic shell effects can run in the wrong workspace

plugins/babysitter-unified/per-harness/omp/extensions-index.ts:59 constructs the driver with cwd: process.cwd(), and the CLI bridge at plugins/babysitter-unified/per-harness/omp/extensions-index.ts:62 also runs pi.exec(..., { cwd: process.cwd() }). OMP tool execution provides a runtime context with the active session cwd, but babysitter_drive does not use it. If the extension process cwd differs from the active workspace/session cwd, project-relative shell effects can run and post successful results for the wrong directory.

Please thread the OMP tool/session cwd through babysitter_drive and runCli (for example via the tool execute context), and add a regression where process.cwd() and the active OMP cwd differ.

  1. Major: owner completion does not enforce strict output schemas

plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:801 implements a small hand-rolled JSON Schema subset for babysitter_agent_complete. That path accepts value: unknown, then validates only simple type, enum, required, properties, and array item checks. It ignores strict constraints such as additionalProperties: false, const, oneOf/anyOf/allOf, string/numeric bounds, and patterns. Since this side channel writes the durable immutable output before task:post, schema-invalid owner results can become trusted Babysitter results even when the task dispatch advertised schemaMode: "strict".

Please use a complete schema validator or the SDK validator on this path, and add regressions for rejected additionalProperties: false and another non-trivial schema constraint.

QA

I dispatched qa-dispatch.yml for PR 1580. Run 30867503585 failed before executing QA: actions/checkout@v6 could not find ref fix/omp-deterministic-driver-recovery in a5c-ai/babysitter, so the QA trigger step was skipped. Local focused verification did pass:

  • npm exec -- vitest run src/__tests__/ompDeterministicDriver.regression.test.ts from packages/adapters/extensions: 11/11 passed after building @a5c-ai/atlas
  • npm exec -- tsc --project plugins/babysitter-unified/per-harness/omp/tsconfig.json: passed
  • npm run build --workspace=@a5c-ai/extensions-adapter: passed
  • npm run verify:metadata: passed

Risk Assessment

Risk level: risk:high.

  • Wrong workspace execution: Shell effects could mutate or verify the wrong project. Mitigation: use the active OMP context cwd and test cwd divergence.
  • Invalid durable results: The owner completion side channel can persist schema-invalid values. Mitigation: enforce full schema validation before writing output.json or posting results.
  • Integration/branch risk: The PR body says #1580 is superseded by #1582, which contains this driver commit plus additional integration work. Mitigation: review/merge #1582 instead, or update #1580 to be independently mergeable and address the issues above.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for PR #1580 as currently scoped.

This is the right general direction for OMP, but this standalone PR should not merge. It still has merge-blocking driver correctness and evidence-integrity gaps, QA did not pass, and the PR body says the change is superseded by #1582.

Blockers

  1. Shell effects with io.outputJsonPath can fail before the driver writes output.json

plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:853

shellResultValue() reads taskDef.io.outputJsonPath immediately after a zero-exit shell command. Existing Babysitter shell gates commonly declare tasks/<effectId>/output.json as the driver-owned output path while the shell command itself only writes stdout/stderr. In that normal shape, this throws ENOENT before resolveShellEffect() writes the driver-owned output.json, so successful shell gates can fail before task:post.

Fix: only read command-owned output when that contract is explicit and the file exists. Otherwise persist captured stdout/failure metadata to the driver-owned output.json before posting. Add a regression with io.outputJsonPath pointing at tasks/<effectId>/output.json where the command writes only stdout.

  1. Accepted skill effects are checkpointed as agent effects and fail durable re-entry

plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:187

drive() accepts both agent and skill actions, but prepareAgentEffect() persists checkpoint.kind = "agent". On re-entry, validateCheckpointIdentity() compares that checkpoint kind with the current action kind, so a skill effect becomes an identity mismatch instead of a recoverable pending/completed effect.

Fix: either do not accept skill effects in this driver, or persist the original effect kind separately from the bridge execution kind. Add skill-effect regressions for re-entry before claim, while owned, and after durable output exists.

  1. The durable owner completion channel can persist schema-invalid strict outputs

plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:801

completeAgentOwnerValue() accepts value: unknown, and persistAgentCompletion() trusts a hand-rolled schema subset before writing immutable output.json. The validator ignores additionalProperties: false, const, combinators, string/numeric bounds, patterns, and object-shaped schemas without explicit type: "object". That can turn schema-invalid owner results into trusted effect evidence.

Fix: use the host/SDK schema enforcement or a complete JSON Schema validator in the generated package. Add rejection tests for additionalProperties: false and at least one combinator or string/numeric constraint.

  1. Mutated bridge task envelopes can claim durable ownership

plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:703

parseAgentBridgeInput() only requires a single babysitter-task item with a syntactically valid BABYSITTER_OMP_BRIDGE descriptor. claimAgentToolCall() then checks descriptor identity and model, but not the full immutable task envelope: prompt text, task name, output schema, schema mode, or agent fields. A modified task payload retaining the descriptor can become the retained writer for instructions the driver did not dispatch.

Fix: persist a canonical hash or the full immutable dispatch envelope in the checkpoint and reject tool_call, tool_result, and owner-completion paths unless the full envelope matches. Add mutation/replay tests for prompt, name/agent, schema, and descriptor replay.

Majors

  • plugins/babysitter-unified/per-harness/omp/extensions-index.ts:59: the driver and pi.exec() are bound to process.cwd() instead of the active OMP workspace/session cwd. Project-relative shell effects can run and post success for the wrong directory. Thread the active OMP cwd through babysitter_drive and runCli, and test process.cwd() differing from the active workspace cwd.

  • plugins/babysitter-unified/per-harness/omp/tsconfig.json:7: the new OMP tsconfig includes only extensions-driver.ts, excluding extensions-index.ts where pi.registerTool, pi.zod, pi.exec, and pi.on("tool_call"/"tool_result") are used. Include the entrypoint or add a separate integration typecheck.

  • packages/adapters/extensions/src/targets/adapters/oh-my-pi.ts:89: generated OMP packages still declare @oh-my-pi/pi-coding-agent: "*" while this PR uses specific tool/event APIs. Constrain the peer range to the minimum compatible OMP version and add a generated-package install/import compatibility test.

QA

I dispatched qa-dispatch.yml for PR #1580. Run 30964713982 failed before product scenarios executed: actions/checkout@v6 tried to checkout a5c-ai/babysitter ref fix/omp-deterministic-driver-recovery, but that branch is on the fork panosAthDBX/babysitter. The QA gate did not pass.

Risk Assessment

Risk level: risk:high

  • Normal shell gates can fail before task:post because driver-owned output paths are read before durable output is written. Mitigation: fix shell output ownership and test real process-library shell gate shapes.
  • Skill effects can strand OMP recovery due to checkpoint kind mismatch. Mitigation: preserve original effect kind separately or stop accepting skill in this path, with focused re-entry tests.
  • Schema-invalid owner results can become immutable effect evidence. Mitigation: enforce complete schema validation before writing output.json.
  • Mutated/replayed OMP task payloads can claim ownership for the wrong assignment. Mitigation: validate the full immutable dispatch envelope.
  • Shell effects can execute in the wrong workspace. Mitigation: use the active OMP/session cwd and test cwd divergence.
  • This PR is marked superseded by #1582. Mitigation: review and land the corrected combined integration PR, or make #1580 independently complete before merging.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial review result: requesting changes. This PR is not mergeable standalone: it has driver correctness blockers, missing production-path regression coverage, failed/inconclusive QA, and the PR body says it is superseded by #1582.

Blockers

  1. Shell effects with io.outputJsonPath can fail before the driver writes durable output

plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:849

shellResultValue() reads io.outputJsonPath on a successful shell result before resolveShellEffect() writes the driver-owned durable output artifact. Existing Babysitter shell gates commonly use output paths as run/task artifacts owned by the harness/driver, while the command itself only emits stdout/stderr. Under this PR, ordinary shell effects can throw ENOENT instead of checkpointing and posting.

Fix: only read a command-owned output file when that contract is explicit and the file exists. Otherwise capture stdout/result metadata and write the driver-owned output before task:post. Add a regression with io.outputJsonPath/shell output path where the command exits 0 and writes only stdout.

  1. Skill effects are accepted but become unrecoverable on re-entry

plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:187 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:430 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:911

drive() routes both agent and skill actions through prepareAgentEffect(), but prepareAgentEffect() persists the checkpoint kind as agent. On re-drive, validateCheckpointIdentity() compares checkpoint kind with the current action kind, so a pending/completed skill effect hits an identity mismatch.

Fix: either reject skill effects until implemented, or persist the original effect kind separately from the bridge execution kind. Add regressions for skill re-entry before claim, while owned, and after completed output exists.

  1. Mutated bridge task envelopes can claim durable ownership

plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:203

claimAgentToolCall() parses the BABYSITTER_OMP_BRIDGE descriptor and checks only requested model identity. It does not verify that the complete generated task envelope still matches the original prompt, task name/agent, output schema, or other immutable dispatch fields. A stale or altered task payload with the descriptor can therefore become the retained writer and later commit a result for instructions/schema the driver did not dispatch.

Fix: persist a canonical hash or full immutable dispatch envelope in the checkpoint and reject tool_call, tool_result, and owner-completion paths unless the envelope matches. Add mutation/replay tests for prompt text, task name/agent fields, schema, and descriptor reuse.

  1. Owner completion does not enforce strict output schemas

plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:318 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:801

completeAgentOwnerValue() relies on a hand-rolled validateJsonSchema() subset. It ignores additionalProperties, const, combinators, string/numeric bounds, patterns, and schemas with required/properties but no explicit type: "object". That lets schema-invalid values become immutable trusted effect evidence.

Fix: use a complete JSON Schema validator or the same strict validator used by SDK/host task outputs. Add rejection tests for additionalProperties: false and at least one combinator or scalar constraint.

Majors

  • plugins/babysitter-unified/per-harness/omp/extensions-index.ts:60 / :63: driver and CLI execution use process.cwd(), not the active OMP workspace/session cwd. Relative shell effects can verify or mutate the wrong tree. Thread the OMP execution context cwd into drive/shell/CLI execution and add a cwd-divergence regression.

  • plugins/babysitter-unified/per-harness/omp/tsconfig.json:7: the OMP typecheck includes only extensions-driver.ts; it excludes extensions-index.ts, where pi.registerTool, pi.on("tool_call"), pi.on("tool_result"), pi.zod, and pi.exec are used. Include the entrypoint or add a separate integration typecheck.

  • packages/adapters/extensions/src/targets/adapters/oh-my-pi.ts:90: generated packages still declare @oh-my-pi/pi-coding-agent: "*" while this PR uses newer OMP APIs. Constrain the peer range to the minimum supported API version and add a strict generated-package install/import compatibility test.

  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:71: coverage misses both accepted skill effects and the real shell artifact shape with io.outputJsonPath. Add focused tests for both production paths.

QA

I dispatched qa-dispatch.yml for PR #1580 as Actions run 30964742776 using the process-specified input branch=fix/omp-deterministic-driver-recovery. The run failed in actions/checkout@v6 before scenarios executed: checkout was against a5c-ai/babysitter with ref fix/omp-deterministic-driver-recovery, but the PR head branch is on the fork panosAthDBX/babysitter. The QA trigger/scenario step was skipped, so QA has not passed.

Risk Assessment

Risk level: risk:high.

  • Normal shell effects can fail or post misleading evidence because output artifact ownership is ambiguous. Mitigation: fix shell output ownership, add process-library shell-shape regressions, and canary an OMP process with consecutive shell gates.
  • Skill effects can strand during OMP recovery/re-entry. Mitigation: preserve original effect kind semantics and add skill recovery tests before release.
  • Mutated bridge envelopes can claim ownership and commit results for the wrong assignment. Mitigation: validate a canonical dispatch envelope/hash on every claim/completion path.
  • Schema-invalid owner results can become immutable effect evidence. Mitigation: enforce complete strict schema validation before writing output.json or posting results.
  • Shell effects can run in the wrong workspace. Mitigation: use the active OMP context cwd and test host-cwd/workspace-cwd divergence.
  • This PR is explicitly superseded by #1582. Mitigation: do not merge #1580 standalone; land a corrected combined integration PR after its blockers and QA path are fixed.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes based on the adversarial review process. This PR should not merge standalone: it has merge-blocking driver correctness issues, the QA gate failed before scenarios ran, and the PR body says the change is superseded by #1582.

Blockers

  1. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:853 reads io.outputJsonPath before the driver writes its durable output. Normal Babysitter shell gates commonly declare tasks/<effectId>/output.json as the harness/driver-owned result while the command only writes stdout. Those successful shell effects can fail with ENOENT before checkpoint completion and task:post.

    Fix: only read command-owned output when that contract is explicit and the file exists; otherwise capture stdout/stderr/exit metadata and write the driver-owned output.json before posting. Add a regression with io.outputJsonPath/shell.outputPath where the command exits 0 and only emits stdout.

  2. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:187 accepts skill actions, but prepareAgentEffect() persists the checkpoint as kind: "agent" at line 430. On re-entry, validateCheckpointIdentity() compares the checkpoint kind to the current action kind at line 911, so skill recovery/re-drive fails for a normal effect kind this driver explicitly routes.

    Fix: either stop handling skill effects here or preserve the original effect kind separately from the bridge execution kind. Add skill-effect regressions for re-entry before claim, while owned, and after completed output exists.

  3. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:801 uses a hand-rolled JSON Schema subset for owner completion. It ignores strict constraints such as additionalProperties, const, combinators, string/numeric bounds, patterns, and object schemas without explicit type: "object". Since babysitter_agent_complete can write the immutable durable result and post it, schema-invalid values can become trusted task evidence.

    Fix: use a complete JSON Schema validator or the same SDK/tooling validator used for strict task outputs. Add rejected-value regressions for additionalProperties: false and at least one combinator or scalar constraint.

  4. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:203 / :916 do not bind bridge ownership to the full immutable dispatch envelope. The claim path verifies descriptor fields and model, but a modified task item can retain the descriptor while changing prompt text, task name, schema, or other dispatch fields, then become the retained owner and later commit a result for instructions the driver did not dispatch.

    Fix: persist a canonical hash or full immutable dispatch envelope in the checkpoint and reject tool_call/tool_result/owner-completion paths unless the complete envelope matches. Add mutation/replay regressions for changed prompt text, task name/agent fields, schema, and descriptor reuse.

Major

  • plugins/babysitter-unified/per-harness/omp/extensions-index.ts:59 and :63 use process.cwd() for both driver cwd and pi.exec cwd. If the extension process cwd differs from the active OMP/session workspace, project-relative shell effects can run and post results for the wrong repository. Thread the active OMP/session cwd through babysitter_drive and runCli, and test cwd divergence.

  • plugins/babysitter-unified/per-harness/omp/tsconfig.json:7 includes only extensions-driver.ts, but the new host integration lives in extensions-index.ts via pi.registerTool() and pi.on(...). Include extensions-index.ts in this typecheck or add a generated-package integration typecheck.

  • packages/adapters/extensions/src/targets/adapters/oh-my-pi.ts:89 still emits @oh-my-pi/pi-coding-agent: "*", while this PR depends on newer OMP APIs (pi.registerTool, pi.on, pi.zod, and the expected pi.exec result shape). Constrain the peer dependency to the minimum compatible API version and add a strict generated-package install/import compatibility test.

  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:71 and nearby tests miss the real process-library shell shape (io.outputJsonPath/shell.outputPath) and do not cover action("skill"), even though production drive() accepts skill actions. Add focused regressions for both accepted paths.

QA

I dispatched qa-dispatch.yml for PR #1580 with branch=fix/omp-deterministic-driver-recovery. Run 30964755697 failed before product QA scenarios ran. The failed step was actions/checkout@v6; it checked out repository a5c-ai/babysitter with ref fix/omp-deterministic-driver-recovery and reported that no branch or tag with that name could be found. Treating QA as failed/inconclusive for this gate.

Risk Assessment

Risk level: risk:high.

  • Normal shell effects can fail before checkpoint completion or task:post because outputJsonPath is read before driver-owned output is written. Mitigation: fix output handling and run regressions against existing process-library shell gate shapes; canary a shell-heavy process before release.
  • Skill-based Babysitter runs can become unrecoverable after interruption or restart. Mitigation: preserve original skill identity or reject skill effects until supported, then canary shell + skill + agent workflows.
  • Schema-invalid or mutated-agent results can be persisted as immutable authoritative evidence. Mitigation: enforce complete schema validation and immutable envelope matching before any owner result write/post.
  • Shell effects can execute in the wrong workspace if the OMP extension cwd differs from the active session cwd. Mitigation: use the active OMP/session cwd and log cwd/runDir/workspace in checkpoints for auditability.
  • This PR is explicitly superseded by #1582. Mitigation: review and merge the corrected integrated PR instead of this standalone branch, or update #1580 until it is independently mergeable.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes. The driver direction is right, but this standalone PR is not safe to merge as-is. It has correctness/evidence blockers in the new OMP driver, the PR body says this branch is superseded by #1582, and the required QA dispatch failed before product scenarios ran.

Blockers

  1. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:187 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:426: skill effects are accepted but persisted as kind: "agent" checkpoints. On re-entry, validateCheckpointIdentity() compares the stored checkpoint kind with the current action.kind, so an accepted skill effect becomes an identity mismatch instead of recovering. Either do not accept skill here, or persist the original effect kind separately from the bridge execution kind. Add regressions for skill dispatch, re-drive while unclaimed/owned, and re-drive after completion.

  2. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:849: successful shell effects with io.outputJsonPath are read before the driver writes durable output. Normal process-library shell gates often declare tasks/<effectId>/output.json as the harness-owned artifact while the command only exits or writes stdout. This path throws before writeImmutableJson(output.json) can run, so deterministic continuation fails for ordinary shell tasks. Distinguish command-owned output files from driver-owned result artifacts and add a regression matching the shared shell-gate shape.

  3. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:289 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:801: babysitter_agent_complete can persist values that are invalid under the advertised strict schema. The hand-rolled validator ignores additionalProperties, const, combinators, string/numeric bounds, patterns, and object schemas without explicit type: "object". Use complete schema validation before writing immutable output, with negative tests for these constraints.

  4. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:203 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:703: a mutated bridge task envelope can claim ownership as long as it retains the original descriptor line and model. The driver does not verify the full prompt, task name/agent, output schema, or schema mode against an immutable dispatch envelope. Persist a canonical dispatch hash or full envelope and reject mutated/replayed task items.

Majors

  • plugins/babysitter-unified/per-harness/omp/extensions-index.ts:59: the driver and pi.exec bridge use process.cwd(), not the active OMP workspace/session cwd. Shell effects can verify or mutate the wrong project and still post success. Thread the active cwd through babysitter_drive and add a cwd-divergence regression.

  • plugins/babysitter-unified/per-harness/omp/tsconfig.json:7: the new OMP tsconfig only includes extensions-driver.ts, excluding extensions-index.ts where pi.registerTool, pi.on, pi.zod, and pi.exec are actually used. Include the entrypoint or add generated-package integration typecheck coverage.

  • packages/adapters/extensions/src/targets/adapters/oh-my-pi.ts:89: the generated package still declares @oh-my-pi/pi-coding-agent: "*" despite new runtime API requirements. Constrain the peer range to the minimum supported OMP version and test generated package activation/import compatibility.

  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:71: regression coverage misses accepted production shapes: no action("skill") path and no process-library shell task with declared io.outputJsonPath/shell.outputPath where the command does not create output.json itself.

QA

I dispatched qa-dispatch.yml for PR #1580 as requested. Run 30964775049 failed in actions/checkout@v6 before product QA scenarios ran:

A branch or tag with the name 'fix/omp-deterministic-driver-recovery' could not be found

The PR head is on the fork panosAthDBX/babysitter, so this dispatch shape does not provide passing QA evidence.

Risk Assessment

Risk level: risk:high

  • Skill and shell effects can fail or strand during OMP replay/re-entry. Mitigation: fix skill-kind identity and shell output ownership, then canary an OMP run with shell + skill + agent effects.
  • Schema-invalid or instruction-mutated agent results can become immutable trusted run evidence. Mitigation: enforce complete strict schema validation and immutable dispatch-envelope matching before release.
  • Shell effects can run in the wrong workspace. Mitigation: use active OMP session cwd and test process cwd/workspace cwd divergence.
  • Generated packages can install on unsupported host versions or ship un-typechecked entrypoint wiring. Mitigation: constrain the peer range and typecheck/import-test the generated OMP entrypoint.

Because the PR is also marked superseded by #1582, please land the corrected implementation through the active integrated PR or update #1580 to be independently mergeable with the fixes above.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes based on the adversarial review process.

Current head has addressed the earlier driver correctness blockers I checked: skill re-entry now tracks effectKind, strict output validation uses Zod JSON Schema conversion, bridge envelope mutation is covered, the OMP entrypoint is in the tsconfig, and the generated package constrains the OMP peer range and declares zod.

Major

  • plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:7 imports fromJSONSchema from zod, but root package.json does not declare zod directly where repository source imports it. The lockfile currently provides node_modules/zod through @oh-my-pi/pi-coding-agent, and the generated OMP package correctly declares zod, so this is not the old strict-schema blocker. It is still dependency hygiene risk for local source checks and future hoisting/dependency changes. Please declare zod directly in the repository package metadata that owns this source import, or document why generated-package-only ownership is sufficient for every supported check path.

QA

I dispatched qa-dispatch.yml for PR #1580 as run 31061035511 with branch=fix/omp-deterministic-driver-recovery. The run failed before product QA scenarios executed. actions/checkout@v6 checked out repository: a5c-ai/babysitter with ref: fix/omp-deterministic-driver-recovery, but that branch exists on the fork panosAthDBX/babysitter, not in a5c-ai/babysitter. The QA gate therefore has not passed.

Merge Path

The PR body still says this branch is superseded by #1582. Please either close/leave #1580 superseded and land the corrected integration through #1582, or update #1580 so it is explicitly the intended standalone landing branch and rerun QA against a checkoutable exact head ref.

Risk Assessment

Risk level: risk:high.

  • OMP deterministic effect driving has high blast radius across shell, skill, and agent effects. Mitigation: require a passing QA run against the exact PR head before merge.
  • The current QA workflow cannot checkout this fork branch using the process-specified branch input. Mitigation: dispatch QA with a repository/ref form that can checkout panosAthDBX/babysitter@fix/omp-deterministic-driver-recovery, or push a same-repo branch before dispatch.
  • The PR is explicitly superseded by #1582. Mitigation: land one intended integration vehicle instead of merging this standalone branch with stale coordination metadata.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for PR #1580.

This PR has the right general direction for OMP deterministic driving, and the current head appears to address several earlier review themes: skill identity preservation, strict schema validation via fromJSONSchema, bridge envelope hashing, OMP entrypoint typecheck coverage, cwd threading, peer range metadata, and owner/retry regressions. It still has a merge-blocking shell-gate correctness issue, the required QA dispatch failed before scenarios ran, and the PR body says this standalone PR is superseded by #1582.

Blocker

  1. Failed shell gates are committed as successful Babysitter effects

plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:744-766, plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:969-999, plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1558-1591

resolveShellEffect() builds a failure-shaped output value when the command times out or exits with a code other than expectedExitCode, but it still marks the checkpoint completed and calls postCompletedCheckpoint(). postCompletedCheckpoint() always runs babysitter task:post ... --status ok, so a failed compile/lint/test shell gate can become a resolved_ok effect whose value merely contains { success: false }.

That violates the shell-task contract this repo relies on: expectedExitCode shell tasks are used as hard binary gates where the exit code cannot be negotiated. Many process definitions just await ctx.task(...); they do not inspect a success field because the effect status is supposed to carry pass/fail. Under this driver, OMP can silently continue after a failed deterministic verification command.

Fix: carry shell failure state through the checkpoint and post it as an error (--status error, with stdout/stderr refs preserved), or stop with operator attention without advancing the run. Add regressions for a nonzero exit and a timeout proving the effect does not resolve ok and the driver does not continue as if the gate passed.

Major

  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts: the new tests cover driver-owned output paths, absent command-owned output, hard process timeout termination, owner retention, retries, schema constraints, and cwd handling, but they do not cover failed shell-result posting semantics. Add tests that drive exitCode != expectedExitCode and timedOut=true through OmpDeterministicDriver.drive() and assert task:post is not called with --status ok.

  • PR metadata/body: #1580 is explicitly marked as superseded by #1582, and #1582 says it contains this deterministic-driver commit plus integration observability on top. Please do not merge #1580 standalone unless it is updated to be the active, independently mergeable PR. Prefer applying the shell-failure fix in #1582 and reviewing that combined branch.

QA

I dispatched the required qa-dispatch.yml run for PR #1580 with branch=fix/omp-deterministic-driver-recovery: run 31061094115.

The run failed before product QA scenarios executed. actions/checkout@v6 used repository: a5c-ai/babysitter and ref: fix/omp-deterministic-driver-recovery, then failed with: A branch or tag with the name 'fix/omp-deterministic-driver-recovery' could not be found. The PR head branch is on the fork, so this dispatch shape does not provide passing QA evidence.

Risk Assessment

Risk level: risk:high.

  • Failed deterministic shell gates can be recorded as successful evidence, allowing broken compile/lint/test checks to pass in OMP-driven Babysitter runs. Mitigation: fix shell failure posting semantics, add nonzero/timed-out regressions, and canary an OMP process with an intentionally failing shell gate.
  • This driver owns run evidence integrity across shell checkpoints, task posting, agent ownership, retries, and generated-package runtime behavior. Mitigation: run the focused OMP matrix plus an end-to-end generated-package smoke covering shell success, shell failure, skill, agent, retry, and late-owner paths before merge.
  • #1580 is superseded by #1582, so standalone merge risks landing a partial integration branch. Mitigation: use one active merge vehicle and land the corrected combined integration PR.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes from the adversarial review process.

Findings

Blocker: clean installs fail because the lockfile is out of sync

package.json:137 adds @oh-my-pi/pi-coding-agent: "16.5.2", but a clean checkout of this PR cannot run npm ci. In /tmp/pr1580-babysitter-review, npm ci failed before any tests or typechecks could start because package.json and package-lock.json are not synchronized.

The npm error reports missing lock entries for the new dependency graph, including proxy-agent@8.0.2, agent-base@9.0.0, http-proxy-agent@9.1.0, https-proxy-agent@9.1.0, pac-proxy-agent@9.1.0, proxy-agent-negotiate@1.1.0, quickjs-wasi@2.2.0, and related packages. The checked-in lockfile records @puppeteer/browsers needing proxy-agent >=8.0.1 at package-lock.json:11133, but the actual package paths present in the lock are older top-level entries such as proxy-agent@6.5.0, agent-base@7.1.4, http-proxy-agent@7.0.2, and pac-proxy-agent@7.2.0.

This blocks reproducible CI installs and means the claimed OMP regressions/typecheck cannot be verified from the submitted artifacts.

Fix: regenerate and commit package-lock.json from this exact package.json, then prove npm ci succeeds in a clean checkout before rerunning the focused OMP checks.

Major: this PR is marked superseded by #1582

The PR body says #1580 is superseded by the combined upstream integration PR #1582. Even after the lockfile is fixed, this standalone branch is not a clear merge vehicle unless it is explicitly unsuperseded and made independently approval-ready.

Fix: close or leave #1580 unmerged and review the active combined PR, or update #1580 so it is explicitly intended to merge standalone.

QA

I dispatched qa-dispatch.yml for PR #1580 as Actions run 31061141964 using the process-specified input branch=fix/omp-deterministic-driver-recovery.

The run failed before product QA scenarios executed. actions/checkout@v6 could not find a branch or tag named fix/omp-deterministic-driver-recovery in a5c-ai/babysitter, and the trigger/scenario step was skipped. QA has not passed for this PR.

I also attempted local focused verification in a detached PR worktree, but npm ci failed first due to the lockfile mismatch above. The vitest and OMP typecheck commands therefore could not be started from a clean install.

Risk Assessment

Risk level: risk:high.

  • Clean installs fail before verification, so CI and downstream contributors cannot reproduce the claimed test results. Mitigation: regenerate the lockfile, run npm ci from a clean checkout, then rerun the focused OMP regression and typecheck commands.
  • QA dispatch does not exercise product scenarios for this fork branch shape. Mitigation: fix the dispatch input or run an equivalent QA path against the correct fork ref before approval.
  • The PR is explicitly superseded by #1582, so merging this branch can land a partial integration stack. Mitigation: use one active integration PR as the merge vehicle and keep the stale branch unmerged.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes. I reviewed the current head c967ea49431f0b5ac5e84fbf5469c25f47b3ec19; several earlier review findings appear addressed, but this still is not safe to merge standalone.

Blockers

  1. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1562: distinct command-owned shell output paths silently fall back to stdout when the artifact is missing.

    shellResultValue() treats io.outputJsonPath differently when it resolves outside the driver-owned tasks/<effectId>/output.json, but if that distinct file is absent it returns result.stdout at the ENOENT path. A distinct output path is the command-owned JSON artifact contract. If a zero-exit command fails to create that artifact, posting stdout as the authoritative effect value can turn a broken producer/verification task into a successful result with the wrong type/value.

    Please fail closed for missing distinct io.outputJsonPath/command output artifacts. Only use stdout fallback when the configured output path is the driver-owned output artifact. The current regression at packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733 codifies the unsafe behavior and should instead expect a failure/no task:post.

  2. plugins/babysitter-unified/per-harness/omp/extensions-index.ts:268 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:507: babysitter_agent_complete can persist a result from descriptor knowledge without proving the caller is the retained owner.

    The registered tool forwards only descriptor fields plus value into completeAgentOwnerValue(). That path validates ownerName, dispatchToken, bridgeEnvelopeSha256, etc. against the checkpoint/owner file, but it does not bind the completion to the retained blocking tool call, host-issued agent identity, or any authenticated owner reference. The descriptor is embedded in the generated task prompt and returned as part of the babysitter_drive dispatch JSON, so descriptor possession is not proof that the retained owner produced the value. A parent/non-owner with the copied descriptor can call the completion tool and write immutable output.json before the real owner returns.

    Please pass authenticated caller identity from the tool execution context into the driver and require it to match the retained owner, or remove the direct completion side channel and accept completion only through the authenticated tool_result path. Add a regression where a non-owner direct babysitter_agent_complete call with a copied descriptor is rejected and does not write output.json.

QA

I dispatched qa-dispatch.yml as run 31061137658 with branch=fix/omp-deterministic-driver-recovery and pr_number=1580. It failed before product scenarios executed. actions/checkout@v6 checked out repository a5c-ai/babysitter with ref fix/omp-deterministic-driver-recovery and failed with: A branch or tag with the name 'fix/omp-deterministic-driver-recovery' could not be found. The scenario step was skipped, so QA has not passed.

Risk Assessment

Risk level: risk:high.

  • Shell effects that should produce structured command-owned JSON can be accepted with stdout instead, corrupting durable task evidence. Mitigation: fail closed for missing distinct output artifacts and canary shell gates using both driver-owned and command-owned outputs.
  • Non-owner completion can bypass the retained single-writer model and persist immutable effect output. Mitigation: bind direct completion to host-authenticated owner identity or remove the side channel.
  • This PR is marked superseded by #1582 and still lacks passing fork-aware QA. Mitigation: land the corrected implementation through the integrated PR after these blockers and QA dispatch are fixed.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes from the adversarial review process.

This PR is not safe to merge standalone. The current head c967ea49431f0b5ac5e84fbf5469c25f47b3ec19 still has merge-blocking reproducibility and OMP evidence-integrity issues, the required QA run failed before product scenarios ran, and the PR body says this branch is superseded by #1582.

Blockers

  1. Clean installs fail because the lockfile is out of sync

package.json:137 / package-lock.json

In a detached clean worktree at the PR head, npm ci --ignore-scripts --dry-run fails before any test or typecheck can start because package.json and package-lock.json are not synchronized. npm reports missing lock entries including proxy-agent@8.0.2, agent-base@9.0.0, http-proxy-agent@9.1.0, https-proxy-agent@9.1.0, pac-proxy-agent@9.1.0, proxy-agent-negotiate@1.1.0, and quickjs-wasi@2.2.0.

This blocks reproducible CI installs and invalidates the claimed verification from the submitted artifacts.

Fix: regenerate and commit package-lock.json from this exact package.json, then prove npm ci succeeds in a clean checkout before rerunning the focused OMP checks.

  1. Failed shell gates are committed as successful Babysitter effects

plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:744-766 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:969-999 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1558-1591

resolveShellEffect() creates a failure-shaped value when a shell command times out or exits with a code other than expectedExitCode, but it still marks the checkpoint completed and calls postCompletedCheckpoint(). postCompletedCheckpoint() always runs task:post --status ok, so a failed compile/lint/test shell gate can become a resolved_ok effect whose value merely contains { success: false }.

That breaks the shell-task contract this repo relies on: expectedExitCode shell tasks are hard binary gates, and most processes just await ctx.task(...) rather than inspecting a success field.

Fix: carry shell failure state through the checkpoint and post it as an error, preserving stdout/stderr refs, or stop for operator attention without advancing. Add regressions for nonzero exit and timeout proving the effect does not resolve ok and the driver does not continue as if the gate passed.

  1. Missing distinct command-owned shell output silently falls back to stdout

plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1562-1575 / packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733-775

When io.outputJsonPath resolves outside the driver-owned tasks/<effectId>/output.json, that path is the command-owned JSON artifact contract. If a zero-exit command fails to create that distinct file, shellResultValue() catches ENOENT and returns stdout, then the driver posts that stdout as the authoritative effect value. The new regression currently codifies this unsafe fallback.

Fix: fail closed for missing distinct command-owned output artifacts. Only use stdout fallback when the configured output path is the driver-owned output artifact, and update the regression to expect failure/no successful task:post.

Majors

  • plugins/babysitter-unified/per-harness/omp/extensions-index.ts:268 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:507-524: babysitter_agent_complete is descriptor-based, not authenticated-owner-based. The registered tool forwards descriptor fields plus value; completeAgentOwnerValue() checks those fields against the owner file, but descriptor possession is not proof that the retained blocking tool-call owner is the caller. A non-owner with copied descriptor data can race to persist immutable output before the real owner returns. Pass host-authenticated caller identity from the tool context and require it to match the retained owner, or remove the direct completion side channel and accept completion only through the authenticated tool_result path. Add a copied-descriptor non-owner rejection regression.

  • PR metadata: the body explicitly says #1580 is superseded by #1582. Even after the code blockers are fixed, this PR should not merge standalone unless it is updated to be the intended merge vehicle.

QA

I dispatched qa-dispatch.yml as run 31138473107 with branch=fix/omp-deterministic-driver-recovery and pr_number=1580.

The run failed before product QA scenarios executed. actions/checkout@v6 checked out repository a5c-ai/babysitter with ref fix/omp-deterministic-driver-recovery, then failed with:

A branch or tag with the name 'fix/omp-deterministic-driver-recovery' could not be found

The PR head branch is on the fork panosAthDBX/babysitter, so this dispatch shape does not provide passing QA evidence.

Risk Assessment

Risk level: risk:high

  • Clean install and CI reproducibility are broken before verification starts. Mitigation: fix and commit the lockfile, run npm ci in a clean checkout, then rerun OMP regression/typecheck/build/metadata gates.
  • OMP can record failed compile/lint/test shell gates as successful evidence. Mitigation: post failed shell effects as errors or stop before continuation; canary an OMP process with intentional failing shell gates.
  • Command-owned JSON outputs can be replaced with stdout and still resolve successfully. Mitigation: fail closed for absent distinct output artifacts and test both driver-owned and command-owned output contracts.
  • Descriptor-based direct completion can violate retained single-writer ownership. Mitigation: bind completion to host-authenticated owner identity or remove direct completion, with replay/race tests.
  • #1580 is marked superseded by #1582 and has no passing fork-aware QA. Mitigation: land one intended integration vehicle after these blockers and QA dispatch are fixed.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes from the adversarial review process.

This PR is not safe to merge as a standalone branch. The current head still has deterministic-driver correctness blockers, clean installs fail before verification can start, QA failed before scenarios executed, and the PR body says #1580 is superseded by #1582.

Blockers

  1. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:766 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:969: failed deterministic shell gates are posted as successful Babysitter effects.

    resolveShellEffect() emits a failed progress state when a command times out or exits with a code other than expectedExitCode, but it still marks the checkpoint completed and calls postCompletedCheckpoint(). postCompletedCheckpoint() always invokes task:post --status ok. That means a failed compile, lint, test, or verification shell gate can become resolved_ok with only a { success: false } value, while normal process code that just await ctx.task(...) continues as if the binary gate passed.

    Fix: carry shell failure state through the checkpoint and post failed/timed-out shell effects with --status error, or stop for operator attention without advancing. Add regressions for nonzero exit and timeout proving the effect is not posted with --status ok.

  2. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1569: missing command-owned shell output silently falls back to stdout.

    When io.outputJsonPath points to a distinct command-owned artifact and that file is absent, shellResultValue() catches ENOENT and returns result.stdout. A distinct output path is the structured artifact contract. If the command exits 0 but fails to produce that file, accepting stdout can turn a broken producer or verification task into a successful effect with the wrong type/value.

    Fix: fail closed when a distinct configured output artifact is missing. Keep stdout fallback only for the driver-owned output artifact shape. The current regression at packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733 expects the unsafe behavior and should be inverted.

  3. package.json:137 / package-lock.json:11133: clean installs fail because the lockfile is out of sync.

    I checked a detached clean worktree for this PR head and npm ci --ignore-scripts fails before tests or typechecks can start. npm reports missing lock entries including proxy-agent@8.0.2, agent-base@9.0.0, http-proxy-agent@9.1.0, https-proxy-agent@9.1.0, pac-proxy-agent@9.1.0, proxy-agent-negotiate@1.1.0, quickjs-wasi@2.2.0, and related packages.

    Fix: regenerate and commit package-lock.json from this exact package.json, then prove npm ci succeeds in a clean checkout before rerunning the focused OMP checks.

Major

  • The PR body says this branch is superseded by #1582. Even after the code blockers are fixed, #1580 is not a clear merge vehicle unless it is explicitly updated to be independently mergeable. Prefer landing the corrected implementation through the active combined PR, or update this PR's metadata and scope so it is the intended standalone branch.

QA

I dispatched qa-dispatch.yml as run 31138499456 with branch=fix/omp-deterministic-driver-recovery and pr_number=1580. The run failed in actions/checkout@v6 before product scenarios executed:

A branch or tag with the name 'fix/omp-deterministic-driver-recovery' could not be found

The scenario trigger step was skipped, so QA has not passed for this PR.

Risk Assessment

Risk level: risk:high

  • Failed compile/lint/test shell gates can be recorded as successful durable Babysitter evidence. Mitigation: fix shell failure posting semantics, add nonzero/timeout regressions, and canary an OMP run with intentionally failing shell gates.
  • Command-owned structured output contracts can be silently replaced by stdout, corrupting downstream task evidence. Mitigation: fail closed for missing distinct output artifacts and add generated-package smoke coverage for both driver-owned and command-owned outputs.
  • Clean installs currently fail, so CI and reviewers cannot reproduce the claimed checks from submitted artifacts. Mitigation: regenerate the lockfile and keep npm ci as a required gate.
  • This PR is marked superseded by #1582, so standalone merge can land a partial integration stack. Mitigation: use one active merge vehicle and rerun QA against the exact intended head.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes from the adversarial review process.

Blockers

  1. package.json:137: clean installs still fail because package-lock.json is out of sync with the added @oh-my-pi/pi-coding-agent: "16.5.2" dependency. In a detached checkout of the PR head c967ea49431f0b5ac5e84fbf5469c25f47b3ec19, npm ci --ignore-scripts --dry-run fails before tests can start. npm reports missing lock entries including proxy-agent@8.0.2, agent-base@9.0.0, http-proxy-agent@9.1.0, https-proxy-agent@9.1.0, pac-proxy-agent@9.1.0, proxy-agent-negotiate@1.1.0, and quickjs-wasi@2.2.0. Please regenerate and commit the lockfile from this exact package.json, then prove npm ci succeeds in a clean checkout before rerunning the OMP checks.

  2. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:744 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:969: failed shell gates are still committed as successful Babysitter effects. resolveShellEffect() creates a failure-shaped value for timeout or exitCode !== expectedExitCode, but still marks the checkpoint completed and calls postCompletedCheckpoint(), which always runs task:post --status ok. Expected-exit-code shell tasks are hard verification gates; process code usually awaits the effect status and does not inspect a nested success: false value. Please carry shell failure state through the checkpoint and post with --status error, or stop for operator attention. Add nonzero-exit and timeout regressions proving no --status ok post happens.

  3. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1562: missing command-owned shell output artifacts silently fall back to stdout. When io.outputJsonPath points outside the driver-owned tasks/<effectId>/output.json, that path is the command-owned JSON artifact contract. If the command exits 0 but does not create that artifact, returning stdout can commit the wrong type/value as authoritative effect evidence. Please fail closed for missing distinct output artifacts and keep stdout fallback only for the driver-owned output artifact shape.

  4. plugins/babysitter-unified/per-harness/omp/extensions-index.ts:268 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:507: babysitter_agent_complete is not bound to an authenticated retained owner. The registered tool ignores tool-call/caller context and forwards only descriptor fields plus value; completeAgentOwnerValue() checks those descriptor fields against the owner file, but descriptor possession is not proof that the retained blocking babysitter-task owner produced the value. The descriptor is embedded in the generated assignment. A parent or non-owner with copied descriptor fields can persist immutable output before the real owner returns. Please pass authenticated caller/owner identity into the driver and require it to match, or remove the direct completion side channel and accept completion only through the authenticated tool_result path. Add a copied-descriptor non-owner regression.

Majors

  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733: the test currently codifies the unsafe missing-command-output behavior by expecting stdout fallback for a distinct io.outputJsonPath. Please invert this test so it expects a fail-closed result/no successful post.

  • PR metadata: the body still says #1580 is superseded by #1582. Even after the blockers are fixed, this PR is not a clear standalone merge vehicle unless it is explicitly updated to be independently mergeable, or the corrected implementation lands through #1582 instead.

QA

I dispatched qa-dispatch.yml as run 31138530923 with branch=fix/omp-deterministic-driver-recovery and pr_number=1580. It failed before product scenarios executed. actions/checkout@v6 used repository: a5c-ai/babysitter and ref: fix/omp-deterministic-driver-recovery, then failed with:

A branch or tag with the name 'fix/omp-deterministic-driver-recovery' could not be found

The trigger/scenario step was skipped, so QA has not passed.

Risk Assessment

Risk level: risk:high.

  • Failed compile/lint/test shell gates can become resolved_ok Babysitter evidence. Mitigation: post failed shell executions as errors or stop, add nonzero/timeout regressions, and canary an OMP run with an intentionally failing shell gate.
  • Structured command-owned outputs can be replaced by stdout when the artifact is missing. Mitigation: fail closed for missing distinct output paths and test driver-owned and command-owned output contracts separately.
  • Non-owner descriptor possession can bypass single-writer ownership and write immutable agent output. Mitigation: authenticate direct completion against the retained owner identity or remove the direct completion path.
  • Clean installs cannot reproduce the claimed verification. Mitigation: fix the lockfile, run npm ci from a clean checkout, then rerun focused OMP regressions, OMP typecheck, adapter build, metadata verification, and fork-aware QA against the exact PR head.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes from the adversarial review process.

This PR is not mergeable as a standalone branch. Current head still has merge-blocking evidence-integrity issues in the OMP driver, clean installs fail, QA failed before scenarios ran, and the PR body says #1580 is superseded by #1582.

Blockers

  1. package.json:137: clean installs fail because package-lock.json is out of sync with the new @oh-my-pi/pi-coding-agent: "16.5.2" dependency.

    In a detached PR worktree, npm ci --ignore-scripts --dry-run fails before any verification can start: npm reports package.json and package-lock.json are not synchronized, with missing lock entries including proxy-agent@8.0.2, agent-base@9.0.0, http-proxy-agent@9.1.0, https-proxy-agent@9.1.0, pac-proxy-agent@9.1.0, proxy-agent-negotiate@1.1.0, and quickjs-wasi@2.2.0. The submitted artifacts therefore cannot reproduce the claimed focused checks from a clean checkout.

    Fix: regenerate and commit package-lock.json from this exact package.json, prove npm ci succeeds in a clean checkout, then rerun the OMP regression/typecheck suite.

  2. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:766: failed or timed-out shell gates are still posted as successful Babysitter effects.

    resolveShellEffect() writes a failure-shaped value for timeout/non-expected exit, marks the checkpoint state: "completed", then calls postCompletedCheckpoint(). postCompletedCheckpoint() always invokes babysitter task:post ... --status ok at plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:982-989. That violates the shell-task contract this repo relies on: expectedExitCode shell tasks are hard compile/lint/test gates, not soft payloads callers are expected to inspect.

    Fix: carry shell failure state through the checkpoint and post it as --status error with stdout/stderr refs, or stop for operator attention without advancing the run. Add regressions for nonzero exit and timeout proving no --status ok post occurs.

  3. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1569: missing command-owned shell output artifacts silently fall back to stdout.

    When io.outputJsonPath points outside the driver-owned tasks/<effectId>/output.json, that is a command-owned JSON artifact contract. If the file is absent, the current ENOENT path returns result.stdout, so a broken producer can still post arbitrary stdout as authoritative task output. The test at packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733 currently codifies this unsafe behavior.

    Fix: fail closed for missing distinct command-owned output paths. Only use stdout fallback for the driver-owned output artifact shape, and rewrite the regression to expect no successful post.

  4. plugins/babysitter-unified/per-harness/omp/extensions-index.ts:268 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:507: babysitter_agent_complete accepts descriptor knowledge without proving the caller is the retained owner.

    The registered tool forwards descriptor fields plus value into completeAgentOwnerValue(). That method validates ownerName/dispatchToken/invocationKey against the owner file, but it does not receive or compare the caller toolCallId, host-issued agent identity, or any authenticated retained-owner reference. The owner file stores toolCallId at plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:143-153, and claim writes it at plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:387-396, but direct completion does not use it. Because the descriptor is embedded in the generated prompt and returned in dispatch JSON, descriptor possession is not proof that the retained blocking owner produced the final value.

    Fix: bind direct completion to authenticated caller identity/tool-call context, or remove the side channel and accept completion only through the authenticated tool_result path. Add a regression where non-owner direct completion with a copied descriptor is rejected and does not write output.json.

Majors

  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733: test coverage currently protects the unsafe missing-output fallback. Rewrite it to assert operator attention or an error/no-ok-post for a missing distinct command-owned output file.

  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts: there is no drive-level regression for failed shell-result posting semantics. Add tests for exitCode != expectedExitCode and timedOut=true through OmpDeterministicDriver.drive() that assert task:post is not called with --status ok.

  • PR metadata: the body still says #1580 is superseded by #1582. Do not merge this standalone branch unless it is updated to be the intended merge vehicle; otherwise land the corrected implementation through #1582.

QA

I dispatched qa-dispatch.yml as run 31138493674 with branch=fix/omp-deterministic-driver-recovery and pr_number=1580. It failed before product scenarios executed. actions/checkout@v6 used repository a5c-ai/babysitter with ref fix/omp-deterministic-driver-recovery and failed because that branch/tag was not found. The PR branch is on the fork, so this process has no passing QA evidence.

Risk Assessment

Risk level: risk:high.

  • Failed deterministic shell verification can be recorded as successful evidence. Mitigation: fix shell failure posting semantics, add nonzero/timeout regressions, and canary an OMP process with an intentionally failing shell gate.
  • Command-owned JSON artifacts can be absent while stdout is accepted as authoritative output. Mitigation: fail closed for missing distinct output paths and test both driver-owned and command-owned output contracts.
  • Non-owner direct completion can bypass retained single-writer ownership and persist immutable evidence. Mitigation: authenticate the completion caller against the retained owner or remove direct completion.
  • The submitted branch cannot be clean-installed or QA-validated as-is. Mitigation: synchronize the lockfile, run npm ci, and dispatch fork-aware QA against the exact head ref before approval.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes from the adversarial review process.

This PR is not safe to merge as a standalone branch. It still has merge-blocking OMP driver evidence-integrity issues, clean installs fail from the submitted artifacts, QA did not run, and the PR body says this branch is superseded by #1582.

Blockers

  1. package.json:137: clean installs fail because package-lock.json is not synchronized with the new @oh-my-pi/pi-coding-agent: "16.5.2" dependency. On detached PR head c967ea49431f0b5ac5e84fbf5469c25f47b3ec19, npm ci --ignore-scripts --dry-run exits before verification starts with missing lock entries including proxy-agent@8.0.2, agent-base@9.0.0, http-proxy-agent@9.1.0, https-proxy-agent@9.1.0, pac-proxy-agent@9.1.0, proxy-agent-negotiate@1.1.0, and quickjs-wasi@2.2.0. Regenerate and commit the lockfile from this exact package.json, then prove npm ci succeeds in a clean checkout.

  2. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:766: failed or timed-out shell gates are still posted as successful Babysitter effects. resolveShellEffect() records failed progress for timeout/non-expected exit, but then calls postCompletedCheckpoint(); that method hard-codes task:post --status ok at plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:982. Expected-exit-code shell tasks are hard compile/lint/test gates, not soft payloads downstream process code must inspect. Carry shell failure state through the checkpoint and post with --status error, or stop for operator attention, and add nonzero-exit/timeout regressions proving no --status ok post occurs.

  3. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1569: missing distinct command-owned shell output artifacts silently fall back to stdout. When io.outputJsonPath points outside the driver-owned tasks/<effectId>/output.json, that path is the structured artifact contract. If the command exits 0 but does not create it, returning stdout can commit the wrong type/value as authoritative evidence. Fail closed for missing distinct output paths and keep stdout fallback only for the driver-owned output artifact shape.

  4. plugins/babysitter-unified/per-harness/omp/extensions-index.ts:268 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:511: babysitter_agent_complete is descriptor-based, not authenticated-owner-based. The registered tool ignores _toolCallId and forwards descriptor fields plus value; completeAgentOwnerValue() checks ownerName/dispatchToken, but not the retained owner toolCallId or any host-authenticated caller identity. A non-owner with copied descriptor fields can race to persist immutable output before the real retained owner returns. Bind direct completion to authenticated owner identity, or remove the side channel and accept completion only through the authenticated tool_result path. Add a copied-descriptor non-owner rejection regression.

Majors

  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733: the test currently codifies the unsafe missing-output fallback by expecting stdout to be posted for an absent distinct io.outputJsonPath. Invert it to expect fail-closed behavior/no successful post.
  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts: add drive-level coverage for failed shell-result posting semantics: exitCode !== expectedExitCode and timedOut=true must not call task:post --status ok.
  • PR metadata: #1580 still says it is superseded by #1582. Do not merge this standalone branch unless it is explicitly updated to be the intended merge vehicle; otherwise land the corrected implementation through #1582.

QA

I dispatched qa-dispatch.yml as run 31230598081 with branch=fix/omp-deterministic-driver-recovery and pr_number=1580. It failed before product scenarios executed: actions/checkout@v6 could not find the branch/tag fix/omp-deterministic-driver-recovery in a5c-ai/babysitter. The scenario trigger step was skipped, so QA has not passed.

Risk Assessment

Risk level: risk:high.

  • Failed compile/lint/test shell gates can become resolved_ok Babysitter evidence. Mitigation: post failed shell executions as errors or stop, add nonzero/timeout regressions, and canary an OMP process with an intentionally failing shell gate.
  • Structured command-owned outputs can be replaced by stdout when the artifact is missing. Mitigation: fail closed for missing distinct output paths and test driver-owned and command-owned output contracts separately.
  • Non-owner descriptor possession can bypass retained single-writer ownership and persist immutable evidence. Mitigation: authenticate the completion caller against the retained owner or remove direct completion.
  • The submitted branch cannot be clean-installed or QA-validated as-is. Mitigation: synchronize the lockfile, run npm ci, and dispatch fork-aware QA against the exact intended head ref before approval.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes from the adversarial review process.

This PR is not safe to merge as a standalone branch. The submitted head still has merge-blocking evidence-integrity issues in the OMP driver, clean installs fail before verification can start, QA failed before scenarios ran, and the PR body says #1580 is superseded by #1582.

Blockers

  1. package.json:137: clean installs fail because package-lock.json is out of sync with the added @oh-my-pi/pi-coding-agent: "16.5.2" dependency.

    In a detached checkout of PR head c967ea49431f0b5ac5e84fbf5469c25f47b3ec19, npm ci --ignore-scripts --dry-run fails before tests can start. npm reports missing lock entries including proxy-agent@8.0.2, agent-base@9.0.0, http-proxy-agent@9.1.0, https-proxy-agent@9.1.0, pac-proxy-agent@9.1.0, proxy-agent-negotiate@1.1.0, and quickjs-wasi@2.2.0.

    Please regenerate and commit the lockfile from this exact package.json, prove npm ci succeeds in a clean checkout, then rerun the OMP regression/typecheck suite.

  2. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:766: failed or timed-out shell gates are still posted as successful Babysitter effects.

    resolveShellEffect() records a failure-shaped value when a command times out or exits with a code other than expectedExitCode, but it still marks the checkpoint completed and calls postCompletedCheckpoint(). postCompletedCheckpoint() always invokes task:post --status ok at plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:982-989. Expected-exit-code shell tasks are hard compile/lint/test gates; callers generally await the effect status and do not inspect a nested success: false payload.

    Please carry shell failure state through the checkpoint and post failed/timed-out shell effects with --status error, or stop for operator attention without advancing. Add nonzero-exit and timeout regressions proving no --status ok post happens.

  3. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1569: missing command-owned shell output artifacts silently fall back to stdout.

    When io.outputJsonPath points outside the driver-owned tasks/<effectId>/output.json, that path is the command-owned JSON artifact contract. If the command exits 0 but does not create that file, the current ENOENT branch returns stdout and can commit the wrong type/value as authoritative effect evidence. The test at packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733 currently codifies this unsafe behavior.

    Please fail closed for missing distinct command-owned output artifacts. Keep stdout fallback only for the driver-owned output artifact shape, and invert that regression.

  4. plugins/babysitter-unified/per-harness/omp/extensions-index.ts:268 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:507: babysitter_agent_complete is descriptor-based rather than authenticated-owner based.

    The registered tool ignores the host toolCallId/caller context and forwards only descriptor fields plus value. completeAgentOwnerValue() checks descriptor data against the owner file, but descriptor possession is not proof that the retained blocking owner produced the value. A non-owner with copied descriptor fields can persist immutable output before the real owner returns.

    Please bind direct completion to an authenticated caller/tool-call identity that matches the retained owner, or remove this side channel and accept completion only through the authenticated tool_result path. Add a copied-descriptor non-owner rejection regression.

Majors

  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733: coverage currently protects the unsafe missing-output fallback. Rewrite it to expect operator attention or an error/no-ok-post for a missing distinct command-owned output file.
  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts: add drive-level regressions for exitCode != expectedExitCode and timedOut=true proving task:post is not called with --status ok.
  • PR metadata still says #1580 is superseded by #1582. Even after the blockers are fixed, this PR is not a clear standalone merge vehicle unless it is updated to be independently mergeable, or the corrected implementation lands through #1582.

QA

I dispatched qa-dispatch.yml as run 31230616265 with branch=fix/omp-deterministic-driver-recovery and pr_number=1580. It failed before product scenarios executed. actions/checkout@v6 looked for repository branch/tag fix/omp-deterministic-driver-recovery and failed with:

A branch or tag with the name 'fix/omp-deterministic-driver-recovery' could not be found

The scenario trigger step was skipped, so QA has not passed.

Risk Assessment

Risk level: risk:high

  • Failed deterministic shell verification can be recorded as successful Babysitter evidence. Mitigation: fix shell failure posting semantics, add nonzero/timeout regressions, and canary an OMP process with an intentionally failing shell gate.
  • Command-owned JSON artifacts can be absent while stdout is accepted as authoritative output. Mitigation: fail closed for missing distinct output paths and test driver-owned and command-owned output contracts separately.
  • Non-owner descriptor possession can bypass retained single-writer ownership and persist immutable evidence. Mitigation: authenticate the completion caller against the retained owner or remove direct completion.
  • The submitted branch cannot be clean-installed or QA-validated as-is. Mitigation: synchronize the lockfile, run npm ci, and dispatch fork-aware QA against the exact PR head before approval.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes from the adversarial review process.

This PR is not safe to merge as a standalone branch. Current head still has merge-blocking evidence-integrity bugs in the OMP driver, clean installs fail, QA failed before scenarios ran, and the PR body says #1580 is superseded by #1582.

Blockers

  1. package.json:137: clean installs fail because package-lock.json is not synchronized with the added @oh-my-pi/pi-coding-agent: "16.5.2" dependency.

    In a detached checkout of PR head c967ea49431f0b5ac5e84fbf5469c25f47b3ec19, npm ci --ignore-scripts --dry-run fails before tests can start. npm reports missing lock entries including proxy-agent@8.0.2, agent-base@9.0.0, http-proxy-agent@9.1.0, https-proxy-agent@9.1.0, pac-proxy-agent@9.1.0, proxy-agent-negotiate@1.1.0, and quickjs-wasi@2.2.0.

    Fix: regenerate and commit package-lock.json from this exact package.json, prove npm ci succeeds in a clean checkout, then rerun the focused OMP checks.

  2. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:766: failed or timed-out shell gates are still posted as successful Babysitter effects.

    resolveShellEffect() emits failed progress for timeout or exitCode !== expectedExitCode, but still marks the checkpoint completed and calls postCompletedCheckpoint(). postCompletedCheckpoint() always invokes task:post --status ok at plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:982-989. Expected-exit-code shell tasks are hard verification gates; process code usually awaits effect status and does not inspect a nested success: false value.

    Fix: carry shell failure state through the checkpoint and post failed/timed-out shell effects with --status error, or stop for operator attention. Add drive-level regressions for nonzero exit and timeout proving no --status ok post occurs.

  3. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1569: missing command-owned shell output artifacts silently fall back to stdout.

    When io.outputJsonPath points to a distinct command-owned artifact and that file is absent, shellResultValue() catches ENOENT and returns result.stdout. A distinct output path is the structured artifact contract. If the command exits 0 but fails to produce that file, accepting stdout can commit the wrong type/value as authoritative effect evidence.

    Fix: fail closed when a distinct configured command-owned output artifact is missing. Keep stdout fallback only for the driver-owned output artifact shape.

  4. plugins/babysitter-unified/per-harness/omp/extensions-index.ts:268 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:507: babysitter_agent_complete is descriptor-based and not bound to the authenticated retained owner.

    The registered tool ignores tool-call/caller context and forwards descriptor fields plus value to completeAgentOwnerValue(). That method checks descriptor fields against agent-owner.json, but it does not compare the completing caller to owner.toolCallId or any host-authenticated identity. Descriptor possession is not proof that the retained blocking owner produced the final value, so a copied descriptor can race to persist immutable output before the actual owner returns.

    Fix: pass authenticated caller/tool-call identity into direct completion and require it to match the retained owner, or remove the direct completion side channel and accept completion only through the authenticated tool_result path. Add a copied-descriptor non-owner regression.

Majors

  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733: regression coverage codifies the unsafe missing-output fallback. The test expects a successful post of stdout when a distinct declared output artifact is missing. Invert this to expect operator attention or an error/no-ok-post for the missing command-owned artifact.

  • PR metadata: the body says #1580 is superseded by #1582. Even after the blockers are fixed, this standalone PR is not a clear merge vehicle unless it is updated to be independently mergeable, or the corrected implementation lands through #1582 instead.

QA

I dispatched qa-dispatch.yml as run 31230621745 with branch=fix/omp-deterministic-driver-recovery and pr_number=1580. It failed before product scenarios executed. actions/checkout@v6 used repository a5c-ai/babysitter with ref fix/omp-deterministic-driver-recovery and failed because that branch/tag was not found. The scenario trigger step was skipped, so QA has not passed for this PR.

Risk Assessment

Risk level: risk:high.

  • Failed compile/lint/test gates can be recorded as successful Babysitter evidence. Mitigation: post shell failures as task errors, add nonzero/timeout regressions, and canary an OMP run with an intentionally failing shell gate.
  • Structured command-owned output contracts can be replaced by arbitrary stdout. Mitigation: fail closed for missing distinct output paths and test driver-owned vs command-owned output separately.
  • Non-owner descriptor possession can bypass retained single-writer ownership. Mitigation: authenticate direct completion against the retained owner identity or remove the side channel, with copied-descriptor race tests.
  • The submitted branch cannot be clean-installed or QA-validated as-is. Mitigation: synchronize the lockfile and dispatch fork-aware QA against the exact intended PR head.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes from the adversarial review process.

This PR is not safe to merge as a standalone branch. Current head c967ea49431f0b5ac5e84fbf5469c25f47b3ec19 still has merge-blocking driver evidence-integrity issues, clean installs fail before verification can start, QA failed before scenarios executed, and the PR body says #1580 is superseded by #1582.

Blockers

  1. package.json:137: clean installs fail because package-lock.json is out of sync with the added @oh-my-pi/pi-coding-agent: "16.5.2" dependency.

    In a detached checkout of the PR head, npm ci --ignore-scripts --dry-run fails before any verification can start. npm reports missing lock entries including proxy-agent@8.0.2, agent-base@9.0.0, http-proxy-agent@9.1.0, https-proxy-agent@9.1.0, pac-proxy-agent@9.1.0, proxy-agent-negotiate@1.1.0, quickjs-wasi@2.2.0, and related packages.

    Fix: regenerate and commit package-lock.json from this exact package.json, prove npm ci succeeds in a clean checkout, then rerun the OMP regression/typecheck/build/metadata gates.

  2. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:744 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:766 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:982: failed or timed-out shell gates are still posted as successful Babysitter effects.

    resolveShellEffect() computes a failed progress state when timedOut is true or exitCode !== expectedExitCode, but still marks the checkpoint completed and calls postCompletedCheckpoint(). postCompletedCheckpoint() always invokes task:post --status ok. Expected-exit-code shell tasks are hard compile/lint/test gates; downstream process code normally awaits the effect status and will continue as if the gate passed.

    Fix: carry shell failure state through the checkpoint and post failed/timed-out shell executions with --status error, or stop for operator attention without advancing. Add drive-level regressions for nonzero exit and timeout proving no --status ok post occurs.

  3. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1562: missing distinct command-owned shell output artifacts silently fall back to stdout.

    When io.outputJsonPath points outside the driver-owned tasks/<effectId>/output.json, that path is the command-owned JSON artifact contract. If the zero-exit command does not create that file, the current ENOENT path returns stdout and can commit the wrong type/value as authoritative effect evidence.

    Fix: fail closed for missing distinct command-owned output paths. Keep stdout fallback only for the driver-owned output artifact shape.

  4. plugins/babysitter-unified/per-harness/omp/extensions-index.ts:268 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:507: babysitter_agent_complete is descriptor-based, not authenticated-owner-based.

    The registered tool forwards descriptor fields plus value; completeAgentOwnerValue() checks those fields against the owner file, but descriptor possession is not proof that the retained blocking owner produced the value. The owner file stores toolCallId, but direct completion does not receive or compare a host-authenticated caller/tool-call identity.

    Fix: bind direct completion to authenticated retained owner identity/toolCallId, or remove the side channel and accept completion only through the authenticated tool_result path. Add a copied-descriptor non-owner regression.

Majors

  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733: the new regression currently protects the unsafe missing-command-output behavior by expecting stdout fallback and successful completion. Invert it to expect operator attention or an error/no successful post.
  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts: add drive-level coverage for exitCode !== expectedExitCode and timedOut=true proving task:post --status ok is not called.
  • PR metadata still says #1580 is superseded by #1582. Do not merge this standalone branch unless it is updated to be the intended merge vehicle; otherwise land the corrected implementation through #1582.

QA

I dispatched qa-dispatch.yml as run 31230631288 with branch=fix/omp-deterministic-driver-recovery and pr_number=1580.

The run failed before product QA scenarios executed. actions/checkout@v6 used repository a5c-ai/babysitter and ref fix/omp-deterministic-driver-recovery, then failed because that branch/tag was not found. The PR head branch is on the fork panosAthDBX/babysitter, so this dispatch shape provides no passing QA evidence.

Risk Assessment

Risk level: risk:high

  • Failed compile/lint/test shell gates can become resolved_ok Babysitter evidence. Mitigation: post failed shell executions as errors or stop, add nonzero/timeout regressions, and canary an OMP run with intentionally failing shell gates.
  • Structured command-owned outputs can be replaced by stdout when the artifact is missing. Mitigation: fail closed for missing distinct output paths and test driver-owned and command-owned output contracts separately.
  • Non-owner descriptor possession can bypass single-writer ownership and write immutable agent output. Mitigation: authenticate direct completion against the retained owner identity or remove the direct completion path.
  • The submitted branch cannot be clean-installed or QA-validated as-is. Mitigation: synchronize the lockfile, run npm ci from a clean checkout, then rerun focused OMP regressions, OMP typecheck, adapter build, metadata verification, and fork-aware QA against the exact PR head.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes from the adversarial review process.

This PR is not safe to merge as a standalone branch. Current head still has merge-blocking evidence-integrity bugs in the OMP deterministic driver, QA failed before scenarios ran, and the PR body says #1580 is superseded by #1582.

Blockers

  1. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:766: failed or timed-out shell gates are still posted as successful Babysitter effects.

    resolveShellEffect() computes a failed progress state when timedOut is true or exitCode !== expectedExitCode at lines 759-763, but it still writes a completed checkpoint and calls postCompletedCheckpoint(). postCompletedCheckpoint() always invokes task:post --status ok at lines 982-989. Expected-exit-code shell tasks are hard compile/lint/test gates; downstream process code can continue as if failed verification passed.

    Fix: carry shell failure state through the checkpoint and post failed/timed-out shell executions with --status error, or stop for operator attention before resolving the effect. Add drive-level regressions for nonzero exit and timeout proving no --status ok post occurs.

  2. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1569: missing distinct command-owned shell output artifacts silently fall back to stdout.

    When io.outputJsonPath points to a distinct command-owned artifact, absence of that file should be a contract failure. Current code catches ENOENT and returns result.stdout, allowing arbitrary stdout to become authoritative structured effect evidence. The test added at packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733-774 codifies this unsafe behavior.

    Fix: fail closed when a distinct configured command-owned output artifact is missing. Keep stdout fallback only for the driver-owned tasks/<effectId>/output.json capture shape, and invert the regression to expect operator attention or an error/no ok post.

  3. plugins/babysitter-unified/per-harness/omp/extensions-index.ts:268 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:507: direct owner completion remains descriptor-based rather than authenticated-owner based.

    babysitter_agent_complete receives only descriptor fields and value. completeAgentOwnerValue() verifies descriptor fields against agent-owner.json, but it never receives or compares the host toolCallId or caller identity for the retained owner. A copied descriptor can race the actual retained owner and persist immutable output through the side channel.

    Fix: bind direct completion to a host-authenticated retained owner identity/toolCallId, or remove the direct completion side channel and accept completion only through the authenticated tool_result path. Add a copied-descriptor non-owner rejection regression.

Majors

  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733: regression coverage protects the unsafe missing-output fallback by expecting a successful post of stdout when a distinct declared output artifact is absent. Rewrite it to expect no successful task:post --status ok for that case.
  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts: add drive-level coverage for exitCode !== expectedExitCode and timedOut=true, proving failed shell gates are not posted as ok.
  • PR metadata: the body says #1580 is superseded by #1582, GitHub reports this PR as merge-conflicting, and the current review decision is already changes requested. Do not merge this standalone branch unless it is updated to be the intended merge vehicle; otherwise land the corrected implementation through #1582.

QA

I dispatched qa-dispatch.yml as run 31286623748 with branch=fix/omp-deterministic-driver-recovery and pr_number=1580.

The run failed before product QA scenarios executed. actions/checkout@v6 used repository a5c-ai/babysitter and ref fix/omp-deterministic-driver-recovery, then failed with: A branch or tag with the name 'fix/omp-deterministic-driver-recovery' could not be found. The scenario trigger step was skipped, so QA has not passed for this PR.

Risk Assessment

Risk level: risk:high.

  • Failed compile/lint/test shell gates can become resolved_ok Babysitter evidence. Mitigation: post shell failures as task errors or operator-attention stops, add nonzero/timeout regressions, and canary an OMP run with intentionally failing shell gates.
  • Structured command-owned output contracts can be replaced by stdout when the artifact is missing. Mitigation: fail closed for missing distinct output paths and test driver-owned vs command-owned contracts separately.
  • Non-owner descriptor possession can bypass retained single-writer ownership and write immutable agent output. Mitigation: authenticate direct completion against retained owner identity or remove the direct completion path, with copied-descriptor race tests.
  • The submitted PR is superseded and merge-conflicting. Mitigation: use #1582 or refresh #1580 as the explicit merge vehicle before approval, then rerun focused OMP regressions, OMP typecheck, adapter build, metadata verification, and fork-aware QA against the exact PR head.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes from the adversarial review process.

This PR is not safe to merge as a standalone branch. Current head c967ea49431f0b5ac5e84fbf5469c25f47b3ec19 has merge-blocking evidence-integrity bugs in the OMP driver, clean installs fail before verification can start, QA failed before scenarios executed, and the PR body says #1580 is superseded by #1582.

Blockers

  1. package.json:137: clean installs fail because package-lock.json is out of sync with the added @oh-my-pi/pi-coding-agent: "16.5.2" dependency.

    In a detached checkout of the PR head, npm ci --ignore-scripts --dry-run fails before any verification can start. npm reports missing lock entries including proxy-agent@8.0.2, agent-base@9.0.0, http-proxy-agent@9.1.0, https-proxy-agent@9.1.0, lru-cache@7.18.3, pac-proxy-agent@9.1.0, proxy-from-env@2.1.0, socks-proxy-agent@10.1.0, proxy-agent-negotiate@1.1.0, get-uri@8.0.1, pac-resolver@9.0.1, quickjs-wasi@2.2.0, data-uri-to-buffer@8.0.0, and degenerator@7.0.1.

    Fix: regenerate and commit package-lock.json from this exact package.json, prove npm ci succeeds in a clean checkout, then rerun the OMP regression/typecheck/build/metadata gates.

  2. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:766: failed or timed-out shell gates are still posted as successful Babysitter effects.

    resolveShellEffect() computes a failed progress state when timedOut is true or exitCode !== expectedExitCode, but still marks the checkpoint completed and calls postCompletedCheckpoint(). postCompletedCheckpoint() always invokes task:post --status ok at plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:982-989. Expected-exit-code shell tasks are hard compile/lint/test gates; downstream process code normally awaits the effect status and will continue as if the gate passed.

    Fix: carry shell failure state through the checkpoint and post failed/timed-out shell executions with --status error, or stop for operator attention without advancing. Add drive-level regressions for nonzero exit and timeout proving no --status ok post occurs.

  3. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1569: missing distinct command-owned shell output artifacts silently fall back to stdout.

    When io.outputJsonPath points outside the driver-owned tasks/<effectId>/output.json, that path is the command-owned JSON artifact contract. If the zero-exit command does not create that file, the current ENOENT path returns stdout and can commit the wrong type/value as authoritative effect evidence. The test at packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733 currently codifies this unsafe behavior.

    Fix: fail closed for missing distinct command-owned output paths. Keep stdout fallback only for the driver-owned output artifact shape, and invert that regression.

  4. plugins/babysitter-unified/per-harness/omp/extensions-index.ts:268 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:507: babysitter_agent_complete is descriptor-based, not authenticated-owner-based.

    The registered tool ignores _toolCallId and forwards descriptor fields plus value; completeAgentOwnerValue() checks ownerName, dispatchToken, invocationKey, and effectId, but not the retained owner toolCallId or any host-authenticated caller identity. Descriptor possession is not proof that the retained blocking owner produced the value, so a copied descriptor can race to persist immutable output before the actual owner returns.

    Fix: bind direct completion to authenticated retained owner identity/toolCallId, or remove the side channel and accept completion only through the authenticated tool_result path. Add a copied-descriptor non-owner regression.

Majors

  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733: the regression currently protects the unsafe missing-command-output behavior by expecting stdout fallback and successful completion. Invert it to expect operator attention or an error/no successful post.
  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts: add drive-level coverage for exitCode !== expectedExitCode and timedOut=true proving task:post --status ok is not called.
  • PR metadata still says #1580 is superseded by #1582. Do not merge this standalone branch unless it is updated to be the intended merge vehicle; otherwise land the corrected implementation through #1582.

QA

I dispatched qa-dispatch.yml as run 31286635722 with branch=fix/omp-deterministic-driver-recovery and pr_number=1580.

The run failed before product QA scenarios executed. actions/checkout@v6 used repository a5c-ai/babysitter and ref fix/omp-deterministic-driver-recovery, then failed because that branch/tag was not found. The PR head branch is on the fork panosAthDBX/babysitter, so this dispatch shape provides no passing QA evidence.

Risk Assessment

Risk level: risk:high.

  • Failed compile/lint/test shell gates can become resolved_ok Babysitter evidence. Mitigation: post failed shell executions as errors or stop, add nonzero/timeout regressions, and canary an OMP run with intentionally failing shell gates.
  • Structured command-owned outputs can be replaced by stdout when the artifact is missing. Mitigation: fail closed for missing distinct output paths and test driver-owned and command-owned output contracts separately.
  • Non-owner descriptor possession can bypass single-writer ownership and write immutable agent output. Mitigation: authenticate direct completion against the retained owner identity or remove the direct completion path.
  • The submitted branch cannot be clean-installed or QA-validated as-is. Mitigation: synchronize the lockfile, run npm ci from a clean checkout, then rerun focused OMP regressions, OMP typecheck, adapter build, metadata verification, and fork-aware QA against the exact PR head.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes from the adversarial review process.

This PR is not safe to merge as a standalone branch. Current head c967ea49431f0b5ac5e84fbf5469c25f47b3ec19 still has merge-blocking evidence-integrity issues, clean installs fail, QA failed before scenarios executed, and the PR body says #1580 is superseded by #1582.

Blockers

  1. package.json:137: clean installs fail because package-lock.json is out of sync with the added @oh-my-pi/pi-coding-agent: "16.5.2" dependency.

    In a detached checkout of the PR head, npm ci --ignore-scripts --dry-run fails before any verification can start. npm reports missing lock entries including proxy-agent@8.0.2, agent-base@9.0.0, http-proxy-agent@9.1.0, https-proxy-agent@9.1.0, pac-proxy-agent@9.1.0, proxy-agent-negotiate@1.1.0, and quickjs-wasi@2.2.0.

    Fix: regenerate and commit package-lock.json from this exact package.json, prove npm ci succeeds in a clean checkout, then rerun the OMP regression/typecheck/build/metadata gates.

  2. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:766: failed or timed-out shell gates are still posted as successful Babysitter effects.

    resolveShellEffect() computes a failed progress state when timedOut is true or exitCode !== expectedExitCode, but still marks the checkpoint completed and calls postCompletedCheckpoint(). postCompletedCheckpoint() always invokes task:post --status ok at plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:982.

    Fix: carry shell failure state through the checkpoint and post failed/timed-out shell executions with --status error, or stop for operator attention without advancing. Add drive-level regressions for nonzero exit and timeout proving no --status ok post occurs.

  3. plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1569: missing distinct command-owned shell output artifacts silently fall back to stdout.

    When io.outputJsonPath points outside the driver-owned tasks/<effectId>/output.json, that path is the command-owned JSON artifact contract. If the zero-exit command does not create that file, the current ENOENT path returns stdout and can commit the wrong type/value as authoritative effect evidence. The test at packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733 currently expects this unsafe behavior.

    Fix: fail closed for missing distinct command-owned output paths. Keep stdout fallback only for the driver-owned output artifact shape, and invert that regression.

  4. plugins/babysitter-unified/per-harness/omp/extensions-index.ts:268 / plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:507: babysitter_agent_complete is descriptor-based, not authenticated-owner-based.

    The registered tool ignores the host _toolCallId and forwards only descriptor fields plus value. completeAgentOwnerValue() checks descriptor fields against agent-owner.json, but descriptor possession is not proof that the retained blocking owner produced the value.

    Fix: bind direct completion to authenticated retained owner identity/toolCallId, or remove the direct completion side channel and accept completion only through the authenticated tool_result path. Add a copied-descriptor non-owner regression.

Majors

  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts:733: regression coverage currently protects the unsafe missing-command-output fallback. Rewrite it to expect operator attention or an error/no successful post.
  • packages/adapters/extensions/src/__tests__/ompDeterministicDriver.regression.test.ts: add drive-level coverage for exitCode !== expectedExitCode and timedOut=true proving task:post --status ok is not called.
  • PR metadata still says #1580 is superseded by #1582, and GitHub reports this PR as conflicting. Do not merge this standalone branch unless it is updated to be the intended merge vehicle; otherwise land the corrected implementation through #1582.

QA

I dispatched qa-dispatch.yml as run 31286638815 with branch=fix/omp-deterministic-driver-recovery and pr_number=1580.

The run failed before product QA scenarios executed. actions/checkout@v6 used repository a5c-ai/babysitter and ref fix/omp-deterministic-driver-recovery, then failed because that branch/tag was not found. The scenario trigger step was skipped, so QA has not passed for this PR.

Risk Assessment

Risk level: risk:high.

  • Failed compile/lint/test shell gates can become resolved_ok Babysitter evidence. Mitigation: post failed shell executions as errors or stop, add nonzero/timeout regressions, and canary an OMP run with intentionally failing shell gates.
  • Structured command-owned outputs can be replaced by stdout when the artifact is missing. Mitigation: fail closed for missing distinct output paths and test driver-owned and command-owned output contracts separately.
  • Non-owner descriptor possession can bypass single-writer ownership and write immutable agent output. Mitigation: authenticate direct completion against the retained owner identity or remove the direct completion path.
  • The submitted branch cannot be clean-installed or QA-validated as-is. Mitigation: synchronize the lockfile, run npm ci from a clean checkout, then rerun focused OMP regressions, OMP typecheck, adapter build, metadata verification, and fork-aware QA against the exact PR head.

@a5c-ai a5c-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict review decision: REJECT.

Blockers:

  • package.json:137 - Clean installs fail because package-lock is not synchronized with @oh-my-pi/pi-coding-agent 16.5.2; npm ci --ignore-scripts --dry-run fails with missing lock entries. Fix: regenerate package-lock.json from this package.json change and prove a clean npm ci plus the OMP gates pass.
  • plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:766 - Failed or timed-out shell gates still proceed to postCompletedCheckpoint; postCompletedCheckpoint posts task:post --status ok around lines 982-989. This can mark failed execution as successful. Fix: post --status error or stop before completion, and add regressions for nonzero exits and timeouts.
  • plugins/babysitter-unified/per-harness/omp/extensions-driver.ts:1569 - Missing distinct command-owned output artifacts fall back to stdout. That accepts untrusted/ambiguous output when a distinct configured outputJsonPath is required. Fix: fail closed when distinct outputJsonPath is configured but missing, and invert the test to require rejection.
  • plugins/babysitter-unified/per-harness/omp/extensions-index.ts:268 - babysitter_agent_complete discards _toolCallId and validates only descriptor fields, so direct completion is not authenticated to the retained owner. Fix: bind direct completion to the host-authenticated caller/tool-call identity or remove the side channel.

Major issues:

  • packages/adapters/extensions/src/tests/ompDeterministicDriver.regression.test.ts:733 - The regression currently protects the unsafe missing-output fallback instead of the secure fail-closed behavior. Fix the expectation so missing configured command output is rejected.
  • packages/adapters/extensions/src/tests/ompDeterministicDriver.regression.test.ts:693 - There is no drive-level coverage proving failed shell executions avoid successful posts. Add coverage that nonzero exits/timeouts do not emit successful task:post --status ok completion.
  • PR body - The PR says it is superseded by #1582, which makes this standalone branch the wrong merge vehicle unless the supersession is resolved or this branch is updated to be the intended merge target.

QA: not dispatched; failed/inconclusive because the PR is superseded and already blocked. ApproachCorrect: false.

Risk Assessment:

  • risk:high
  • Impact: this can break clean installs, falsely report failed shell gates as successful, accept unsafe/missing output artifacts, and allow unauthenticated direct completion through a side channel.
  • Mitigations: synchronize package-lock.json, prove clean npm ci and OMP gates, fail closed for missing distinct output artifacts, bind completion to host-authenticated caller/tool-call identity or remove the side channel, add nonzero/timeout and missing-output regressions, and resolve the #1582 supersession before review proceeds.

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

Labels

None yet

Projects

None yet

1 participant