Skip to content

fix: validate advertised slash command names and text selection - #721

Closed
benbrandt wants to merge 2 commits into
mainfrom
fix/command-validation
Closed

benbrandt wants to merge 2 commits into
mainfrom
fix/command-validation

Conversation

@benbrandt

Copy link
Copy Markdown
Member

Summary

Continues the original SDK 2.2 audit, pinned to specification d5e9949c3e6a9cd072adebaa44e6f63289590c17. Later specification changes are excluded.

  • Skip empty and whitespace-bearing skill names when constructing command lists. Keep the existing first-wins deduplication across skill roots and the separate $skill namespace alongside built-ins.
  • Parse the first text block anywhere in a prompt, including after an image. Do not skip an earlier text block to find a later command.
  • Match advertised command spelling and require the slash immediately before the name. Unknown names and name-prefix near misses remain ordinary prompts.
  • Add dual-version wire coverage for filtering, duplicate skills, built-in/skill namespace separation, mixed content, exact name boundaries, and ordinary-prompt forwarding.

Compatibility

/STATUS, / status, and leading-whitespace /status now go to Codex as ordinary prompts instead of invoking /status locally. Advertised command names are lowercase and have no leading whitespace. /mcp RECONNECT still works: argument/subcommand interpretation is unchanged. The old MCP test was narrowed to assert subcommand case-insensitivity rather than an unadvertised uppercase command alias.

Validation

  • npm run typecheck: passed
  • npm run build: passed
  • npm test: 2,381 passed, 39 skipped
  • npm run test:integration: all 19 passed
  • npm run codex-test -- -p "/status" -o summary: end_turn, no model turn
  • git diff --check: passed

Opt-in E2E tests were not run. This PR is independent of #720 and based directly on main. No review Markdown file is included.

benbrandt added a commit that referenced this pull request Nov 2, 2026
## Summary

Continues the original SDK 2.2 audit at specification baseline
`d5e9949c3e6a9cd072adebaa44e6f63289590c17`. No later specification
changes are included.

The missing cancellation-crossing-next-prompt test exposed a real bug:
an old `turn/interrupt` could return `no active turn to interrupt` after
a newer prompt started. Retry eligibility checked only whether *any*
prompt was active, then recomputed the turn ID from the newer prompt's
state. The old cancellation could therefore interrupt work received
after it.

- Bind interrupt retries to the original active-prompt identity and its
registered turn, rather than to the presence of any active prompt.
- Recheck ownership after retry waits and while awaiting child
interruption/pending turn starts.
- Do not remap an explicit late turn ID through a newer prompt's
registered state.
- Keep legitimate same-prompt registration retries, including the
existing shared v1 cancellation behavior.
- Add a v2 wire regression matrix for acknowledgement-before-completion,
completion-before-acknowledgement, and a delayed no-active-turn error.
Earlier queued prompts are cancelled; later prompts run normally and are
never targeted by the old interrupt.

## Validation

- `npm run typecheck`: passed
- `npm run build`: passed
- `npm test`: 2,368 passed, 39 skipped
- `npm run test:integration`: all 19 passed
- `git diff --check`: passed

The late-error regression failed before the implementation fix by
observing an extra interrupt against `turn-2`. Existing v1 cancellation
and retry tests pass unchanged. The real-Codex integration matrix covers
normal cancellation and child ownership; the exact delayed-error race is
injected through the native mock for deterministic wire coverage. Opt-in
E2E tests were not run.

This PR is independently based on main and does not include #720, #721,
or #722. No review Markdown file is included.
benbrandt added a commit that referenced this pull request Nov 2, 2026
## Summary

Continues the original SDK 2.2 audit at specification baseline
`d5e9949c3e6a9cd072adebaa44e6f63289590c17`. No later specification
features are added.

- Extend the existing child configuration/readback owner to publish
verified absolute child `cwd` values as standard `subagent_update`
frames on the immediate parent.
- Do not reset directory fields on announcements or replay. `null`
explicitly means inheritance, not unknown placement. Keep retained
overrides until verified current data replaces them, rather than
temporarily replacing a worktree path with the parent's directory.
- Reuse native child-ID, parent-ID, loaded-state, attachment, and
pending-read checks. Current native settings outrank older readback.
Optional readback does not block child output.
- Omit `additionalDirectories`: Codex does not expose an authoritative
child ACP additional-directory list. Do not infer it from provider IDs,
parent roots, or writable sandbox paths, and do not report `[]` or
`null` as a substitute for unknown data.
- Keep AIR's legacy subagent-event contract unchanged.

## Validation

- `npm run typecheck`: passed
- `npm run build`: passed
- `npm test`: 2,379 passed, 39 skipped
- `npm run test:integration`: all 19 passed; the real-Codex child matrix
verifies directory publication before and after reattachment on both
protocol versions
- `git diff --check`: passed

Dual-version wire coverage includes worktree placement, nested parent
routing, replay without inheritance resets, unavailable current
placement, invalid paths, superseded reads, unload/close, and
foreign/mismatched child readback. Existing lifecycle goldens include
the verified directory updates; these goldens consistently end with a
newline.

Includes the same Linux-aware background-process test probe as
#720/#721: Linux zombie/dead PIDs are exited even while they remain
unreaped. A live process at the close-response boundary still fails the
assertion; no post-close wait is added.

The real-Codex integration matrix directly verifies directory behavior.
Opt-in E2E tests were not run. This PR is independently based on main
and does not include #720 or #721. No review Markdown file is included.
@benbrandt benbrandt closed this Nov 2, 2026
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