Skip to content

fix: inline dependabot auto-merge; test pi + docker-sandbox executors#59

Open
djimit wants to merge 2 commits into
mainfrom
fix/p0-automerge-executor-tests
Open

fix: inline dependabot auto-merge; test pi + docker-sandbox executors#59
djimit wants to merge 2 commits into
mainfrom
fix/p0-automerge-executor-tests

Conversation

@djimit

@djimit djimit commented Jul 15, 2026

Copy link
Copy Markdown
Owner

Summary

P0 items from the APEX validation session (2026-07-15):

  1. Dependabot Auto-Merge fixed — the workflow called a reusable workflow in the private djimit/.github repo, which a public repo cannot do (workflow failed at startup, 0s, on every dependabot PR since June 30). Inlined the job with dependabot/fetch-metadata, auto-merging patch/minor only — same remediation as the earlier CodeQL inline.
  2. pi-executor tests (9 tests) — buildPiArgs env/option contract (deterministic flags, model precedence, tools allowlist), mapPiEvent lifecycle + tool mapping, JSON event parsing, token-usage capture from assistant messages, heuristic fallback classification.
  3. docker-sandbox-executor tests (9 tests) — buildDockerArgs isolation flags (cpu/memory/network/read-only+tmpfs), __-prefixed env filtering, bind mounts, wrapper kind/canExecute delegation, sandbox event bracketing, result passthrough.

Executor coverage goes from 8/10 to 10/10. All 18 new tests pass in 166ms; no production code changed.

Validation

  • npx vitest run pi-executor.test.ts docker-sandbox-executor.test.ts → 18/18 pass
  • npx eslint on both new files → clean

🤖 Generated with Claude Code

- Inline the Dependabot auto-merge job: the reusable workflow in the
  private djimit/.github repo cannot be called from this public repo
  (same root cause as the CodeQL inline). Auto-merges patch/minor only.
- Add pi-executor tests: buildPiArgs env/option contract, mapPiEvent
  lifecycle + tool mapping, JSON parsing, token-usage capture,
  heuristic fallback classification.
- Add docker-sandbox-executor tests: buildDockerArgs isolation flags,
  env filtering, bind mounts, wrapper delegation and event bracketing.

Closes the last two executor coverage gaps (8/10 -> 10/10).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kilo-code-bot

kilo-code-bot Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Kilo Code Review could not run — your account is out of credits.

Add credits or switch to a free model to enable reviews on this change.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces comprehensive unit tests for both the DockerSandboxExecutor and PiExecutor components, ensuring correct command-line argument construction, environment variable forwarding, and event mapping. The review feedback suggests improving type safety in the Docker sandbox tests by using the ExecutorOptions type instead of inline type literals. Additionally, it recommends asserting that CLI flags exist in the arguments array before using indexOf to avoid confusing test failures if a flag is missing.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

import { describe, expect, it } from 'vitest';
import type { Task, ExecutionEventCreateInput } from '@djimitflo/shared';
import { ExecutionEventType, LogLevel } from '@djimitflo/shared';
import type { TaskExecutor, ExecutionSession, ExecutorKind } from '../execution/types';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Import the ExecutorOptions type to ensure type safety and consistency with the TaskExecutor interface.

Suggested change
import type { TaskExecutor, ExecutionSession, ExecutorKind } from '../execution/types';
import type { TaskExecutor, ExecutionSession, ExecutorKind, ExecutorOptions } from '../execution/types';

return task.risk_level !== ('critical' as any);
}

async start(task: Task, options?: { environment?: Record<string, string> }): Promise<ExecutionSession> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Use the imported ExecutorOptions type instead of an inline type literal for the options parameter.

Suggested change
async start(task: Task, options?: { environment?: Record<string, string> }): Promise<ExecutionSession> {
async start(task: Task, options?: ExecutorOptions): Promise<ExecutionSession> {

Comment on lines +84 to +88
expect(args[args.indexOf('--cpus') + 1]).toBe('2.0');
expect(args[args.indexOf('--memory') + 1]).toBe('256m');
expect(args[args.indexOf('--network') + 1]).toBe('none');
expect(args).toContain('--read-only');
expect(args[args.indexOf('--tmpfs') + 1]).toBe('/tmp:size=64m');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Assert that the flags exist in the args array before accessing their values using indexOf. If a flag is missing, indexOf returns -1, which causes the assertion to check args[0] (which is 'run') and produce a highly confusing failure message.

    expect(args).toContain('--cpus');
    expect(args[args.indexOf('--cpus') + 1]).toBe('2.0');
    expect(args).toContain('--memory');
    expect(args[args.indexOf('--memory') + 1]).toBe('256m');
    expect(args).toContain('--network');
    expect(args[args.indexOf('--network') + 1]).toBe('none');
    expect(args).toContain('--read-only');
    expect(args).toContain('--tmpfs');
    expect(args[args.indexOf('--tmpfs') + 1]).toBe('/tmp:size=64m');

const args = DockerSandboxExecutor.buildDockerArgs('sandbox-1', config, 'sh', [], '/host/project');

expect(args).toContain('/host/project:/workspace:rw');
expect(args[args.indexOf('-w') + 1]).toBe('/workspace');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Assert that the -w flag exists in the args array before checking its value using indexOf to ensure clear test failure messages.

Suggested change
expect(args[args.indexOf('-w') + 1]).toBe('/workspace');
expect(args).toContain('-w');
expect(args[args.indexOf('-w') + 1]).toBe('/workspace');

it('prefers options.model over PI_MODEL env', () => {
process.env.PI_MODEL = 'env-model';
const optArgs = buildPiArgs(makeTask(), { model: 'option-model' });
expect(optArgs[optArgs.indexOf('--model') + 1]).toBe('option-model');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Assert that the --model flag exists in the optArgs array before checking its value using indexOf to ensure clear test failure messages.

Suggested change
expect(optArgs[optArgs.indexOf('--model') + 1]).toBe('option-model');
expect(optArgs).toContain('--model');
expect(optArgs[optArgs.indexOf('--model') + 1]).toBe('option-model');

expect(optArgs[optArgs.indexOf('--model') + 1]).toBe('option-model');

const envArgs = buildPiArgs(makeTask());
expect(envArgs[envArgs.indexOf('--model') + 1]).toBe('env-model');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Assert that the --model flag exists in the envArgs array before checking its value using indexOf to ensure clear test failure messages.

Suggested change
expect(envArgs[envArgs.indexOf('--model') + 1]).toBe('env-model');
expect(envArgs).toContain('--model');
expect(envArgs[envArgs.indexOf('--model') + 1]).toBe('env-model');

Comment on lines +98 to +101
expect(args[args.indexOf('--provider') + 1]).toBe('ollama');
expect(args[args.indexOf('--thinking') + 1]).toBe('high');
expect(args[args.indexOf('--tools') + 1]).toBe('read,ls');
expect(args[args.indexOf('--exclude-tools') + 1]).toBe('bash');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Assert that the flags exist in the args array before checking their values using indexOf to ensure clear test failure messages.

    expect(args).toContain('--provider');
    expect(args[args.indexOf('--provider') + 1]).toBe('ollama');
    expect(args).toContain('--thinking');
    expect(args[args.indexOf('--thinking') + 1]).toBe('high');
    expect(args).toContain('--tools');
    expect(args[args.indexOf('--tools') + 1]).toBe('read,ls');
    expect(args).toContain('--exclude-tools');
    expect(args[args.indexOf('--exclude-tools') + 1]).toBe('bash');

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8051f987f1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

github-token: ${{ secrets.GITHUB_TOKEN }}
- name: Enable auto-merge for patch/minor updates
if: steps.metadata.outputs.update-type != 'version-update:semver-major'
run: gh pr merge --auto --squash "$PR_URL"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Use a write-capable trigger for auto-merge

Because this workflow still runs on pull_request, the new gh pr merge --auto step will execute on Dependabot PRs with a read-only token and fail instead of enabling auto-merge. The official dependabot/fetch-metadata README notes that write automation should use pull_request_target or workflow_run, and its auto-merge example uses pull_request_target with contents: write and pull-requests: write.

Useful? React with 👍 / 👎.

…rOptions

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant