Skip to content

test(cli): unflake permission.create after isolated fixtures - #13113

Merged
johnnyeric merged 1 commit into
mainfrom
johnnyeric/fix-httpapi-exerciser-permission-flake
Aug 13, 2026
Merged

test(cli): unflake permission.create after isolated fixtures#13113
johnnyeric merged 1 commit into
mainfrom
johnnyeric/fix-httpapi-exerciser-permission-flake

Conversation

@johnnyeric

@johnnyeric johnnyeric commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Issue

No existing issue. This is a test-only flake that started after #13091 isolated v2.session.permission.create on a fresh project.

Context

#13091 made this scenario cheap enough to run before the default build agent finishes loading. PermissionV2.configured then falls back to deny-all, so the assertion effect === "ask" flakes.

Wait for that agent over GET /api/agent before asserting. The failure message now includes the observed effect so a recurrence is diagnosable. No production change.

Implementation

sessionAfterDefaultAgent polls GET /api/agent until AgentV2.defaultID appears, then the scenario posts the permission request. That list and the .env ask rule are written in the same AgentPlugin transform, so the gate is causally correct for this assertion.

The probe uses the same x-kilo-directory the later POST uses. A workspace/worktree session would boot a different AgentV2 and this wait would not cover it. The wait stays on this scenario rather than the shared runner so the other 300+ scenarios do not pay an extra HTTP poll.

Screenshots / Video

N/A — test-only, no UI change.

How to Test

Manual/local verification

  • Agent: bun run typecheck in packages/opencode/ — pass
  • Agent: bun run script/check-opencode-annotations.ts --worktree — pass
  • Agent: bun run script/httpapi-exercise.ts --mode effect --include v2.session.permission.create --fail-on-skipPASS

Reviewer test steps

  1. From packages/opencode/, run bun run script/httpapi-exercise.ts --mode effect --include v2.session.permission.create
  2. Confirm the scenario passes with effect === "ask"

Blocked checks and substitute verification

  • Full bun run test:httpapi was not re-run after the assertion message change; the isolated v2.session.permission.create scenario was.

Checklist

  • Issue linked above, or exception explained
  • Tests/verification described
  • Screenshots/video included for visual changes, or marked N/A
  • Changeset considered for user-facing changes
  • I personally reviewed the diff and can explain the changes, including any AI-assisted work.

Get in Touch

@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: 278cd7ee2d

ℹ️ 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".

Comment thread packages/server/src/handlers/permission.ts Outdated
Comment thread packages/server/src/handlers/permission.ts Outdated
@kilo-code-bot

kilo-code-bot Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files, incremental)
  • packages/opencode/test/kilocode/server/httpapi-exercise-ready.ts — new sessionAfterDefaultAgent gate polls GET /api/agent with pollWithTimeout (bounded 5s, 20ms interval, clear failure message); uses the same ctx.headers() directory header as the later POST, so the wait covers the same AgentV2 instance. No leaks: each poll iteration is a transient in-process request, no subscriptions or fibers retained
  • packages/opencode/test/server/httpapi-exercise/backend.ts — new request helper reuses the cached no-auth app and capture(..., "full"); request construction matches the existing toRequest shape, Method/RequestSpec types verified in types.ts
  • packages/opencode/test/server/httpapi-exercise/index.ts — scenario seed swap plus a diagnostic assertion message that includes the observed effect

The PR was rewritten since the last review: the previous production-side readiness fix (PluginInternal.Service, the handler wait in packages/server/src/handlers/permission.ts, and the changeset) was fully reverted in favor of this test-only gate. Both previous inline comments refer to removed code and are now outdated; no findings carry forward. Test-only change, so the dropped changeset is correct. The Effect.orDie on the seed matches existing runner helper style (runner.ts:179,181).

Previous Review Summaries (2 snapshots, latest commit bf75e30)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit bf75e30)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files, incremental)
  • packages/core/src/plugin/internal.ts — new PluginInternal.Service (Deferred-based readiness) is a valid LayerNode service identity, wired into locationServices; done fires via Effect.onExit before forkScoped, so the wait completes on success, failure, defect, and interruption
  • packages/server/src/handlers/permission.ts — handler now waits on internal plugin boot completion instead of agent plugin registration
  • packages/core/test/location-layer.test.ts — new live test exercises the real LocationServiceMap path; asserted *.env rule matches agent.ts:119

The previous SUGGESTION (unbounded wait if the agent plugin never registers) is resolved: PluginInternal.wait now resolves on any boot exit — Deferred.done propagates failures to waiters instead of hanging. The codex P1 concern (wait until the batched agent state commits) is also addressed, since the wait covers the entire State.batch. No memory leaks: the Deferred completes exactly once via onExit (including scope-close interruption), and the boot fiber remains forkScoped to the layer lifetime.

Previous review (commit 278cd7e)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 1
Issue Details (click to expand)

SUGGESTION

File Line Issue
packages/server/src/handlers/permission.ts 27 PluginV2.wait("agent") blocks indefinitely if an earlier plugin in PluginInternal.boot fails before AgentPlugin is registered (pre-change behavior was a deny-all response). Consider a timeout or fallback; safe to skip if broken plugin boot is treated as fatal.
Files Reviewed (2 files)
  • .changeset/fix-permission-agent-plugin-race.md - 0 issues (user-facing changeset present and well-worded)
  • packages/server/src/handlers/permission.ts - 1 issue

The core fix is sound: wait("agent") resolves only after AgentPlugin's effect completes, which is what registers the default .env ask rules, so the race with PluginInternal.boot's forked fiber is correctly closed. The 2-line hook in a shared opencode file is minimal and properly marked with kilocode_change, and placing it in the handler avoids the permission → plugin → skill → permission import cycle described in the PR. No memory leaks introduced — waiters are cleaned up via ensuring on completion/interruption.

Fix these issues in Kilo Cloud


Reviewed by kimi-k3 · Input: 160.7K · Output: 6.7K · Cached: 448.5K

Review guidance: REVIEW.md from base branch main

@johnnyeric
johnnyeric marked this pull request as draft August 13, 2026 19:08
@johnnyeric
johnnyeric force-pushed the johnnyeric/fix-httpapi-exerciser-permission-flake branch from bf75e30 to 73de3cf Compare August 13, 2026 19:26
@johnnyeric johnnyeric changed the title fix(cli): wait for agent plugin before permission create test(cli): wait for default agent before permission create Aug 13, 2026
@johnnyeric johnnyeric changed the title test(cli): wait for default agent before permission create test(cli): unflake permission.create after isolated fixtures Aug 13, 2026
@johnnyeric
johnnyeric force-pushed the johnnyeric/fix-httpapi-exerciser-permission-flake branch 2 times, most recently from 0932376 to 2177b47 Compare August 13, 2026 19:41
After #13091 isolated this scenario on a fresh project, permission.create can race AgentPlugin boot and get deny-all. Wait for the default build agent over GET /api/agent before asserting ask.
@johnnyeric
johnnyeric force-pushed the johnnyeric/fix-httpapi-exerciser-permission-flake branch from 2177b47 to df8f689 Compare August 13, 2026 19:49
@johnnyeric
johnnyeric marked this pull request as ready for review August 13, 2026 19:51
@johnnyeric
johnnyeric merged commit e9a3aa0 into main Aug 13, 2026
44 of 47 checks passed
@johnnyeric
johnnyeric deleted the johnnyeric/fix-httpapi-exerciser-permission-flake branch August 13, 2026 21:22
yz-anaconda added a commit that referenced this pull request Aug 14, 2026


Main landed the same httpapi design (git-opt-in fixtures, --shard flag)
and the same permission.create readiness wait. Resolution adopts main's
implementations and keeps this branch's unique work on top: the
--shards child-process orchestrator (adapted to main's zero-based
shard indexing), scenario retry with backoff gated on state-reset,
project.update isolation, and exerciser env stripping. Removed the
superseded agentsReady/probe machinery and this branch's duplicate
round-robin filter (double-sharding silently dropped scenarios).

Validated: coverage, auth, and 4-shard effect passes all green
(315 checks, 0 failures).
t7tran pushed a commit to t7tran/kilocode that referenced this pull request Aug 14, 2026
…13113)

After Kilo-Org#13091 isolated this scenario on a fresh project, permission.create can race AgentPlugin boot and get deny-all. Wait for the default build agent over GET /api/agent before asserting ask.
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.

2 participants