Skip to content

fix: render design sections before the approval question in brainstorming skill#1915

Open
mvanhorn wants to merge 11 commits into
obra:devfrom
mvanhorn:fix/1731-brainstorming-design-preview
Open

fix: render design sections before the approval question in brainstorming skill#1915
mvanhorn wants to merge 11 commits into
obra:devfrom
mvanhorn:fix/1731-brainstorming-design-preview

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

This PR targets the dev branch.

Who is submitting this PR? (required)

Field Value
Your model + version GPT-5.5 (Codex CLI) wrote the diff; Claude Fable 5 orchestrated planning and review
Harness + version Claude Code CLI (current release) driving Codex CLI 0.142.x
All plugins installed codex plugin 1.0.4
Human partner who reviewed this diff @mvanhorn

What problem are you trying to solve?

#1731: users going through the brainstorming skill's design review reach the "Does the design above look right?" approval prompt with nothing rendered above it. The design section never appears. @trautonen traced the root cause: after a Claude Code update, assistant message text that precedes an AskUserQuestion tool call in the same turn is not rendered, so any skill that emits a design section and then immediately asks an interactive question in one turn loses the section. @dch-actaport reproduced the same failure on Junie with Sonnet 4.6, so this is not specific to one harness or model.

What does this PR change?

Adds guidance to the "Presenting the design" flow in skills/brainstorming/SKILL.md: deliver each design section as plain rendered message text in its own turn, and only then gather approval. Never combine section content with an interactive question tool call in the same turn.

Is this change appropriate for the core library?

Yes. The brainstorming skill is core, and the failure mode (pre-tool-call text swallowed by the harness) affects every user of the skill on current Claude Code, independent of project type. No third-party service is involved.

What alternatives did you consider?

  • Restructuring the design-review flow to avoid AskUserQuestion entirely: rejected as a much larger behavioral change to carefully-tuned content for the same effect.
  • Waiting for the harness to restore pre-tool-call rendering: the second report on Junie shows other harnesses behave the same way, so the skill's guidance is the right layer for the fix regardless of any harness change.
  • A note in README/docs instead of SKILL.md: agents don't read the README mid-session; the instruction has to live where the agent's behavior is shaped.

Does this PR contain multiple unrelated changes?

No. One file, one additive instruction (+3/-1).

Existing PRs

Environment tested

Harness (e.g. Claude Code, Cursor) Harness version Model Model version/ID
Not session-tested (see Evaluation) - - -

New harness support (required if this PR adds a new harness)

N/A - no new harness. (Transcript section omitted per template: only required when adding harness support.)

Evaluation

  • Initial prompt: this change came from triaging Design Review not showing the Spec after claude update #1731 directly, not from a failing session of our own. The reproduction and root cause come from the issue thread: @trautonen's diagnosis of pre-tool-call text not rendering, confirmed by @dch-actaport on a second harness (Junie, Sonnet 4.6).
  • Eval sessions after the change: 0. Being upfront about that. The change encodes the issue thread's diagnosed rendering constraint (text before an in-turn interactive tool call is not rendered on current Claude Code) as presentation guidance; we have not run before/after brainstorming eval sessions.
  • Outcome change: expected, not measured - the approval question gets visible section content above it because the section is delivered as its own rendered turn. If you want N-session before/after evals before considering this, say so and we'll run them; the two independent reproductions in the thread are the evidence base today.

Rigor

  • I used superpowers:writing-skills and completed adversarial pressure testing
  • This change was tested adversarially, not just on the happy path - not session-tested; the edge cases were checked by analysis only (revise loop re-presents the section in its own turn each iteration; harnesses that still render pre-tool-call text read the present-then-ask pattern naturally)
  • I did not modify carefully-tuned content (Red Flags table, rationalizations, "human partner" language) - the edit is additive guidance in the presentation flow only; no tuned prose was rewritten

Human review

  • A human has reviewed the COMPLETE proposed diff before submission

Fixes #1731

AI was used for assistance.

arittr and others added 11 commits July 2, 2026 14:56
…s object

Codex auto-discovers a plugin's hooks/hooks.json whenever the Codex
manifest has no `hooks` field: load_plugin_hooks falls back to a
hardcoded DEFAULT_HOOKS_CONFIG_FILE = "hooks/hooks.json" and registers
it. hooks/hooks.json is the Claude Code SessionStart hook, it is tracked
in this repo, and the Codex marketplace installs the whole repo root
(source url "./"), so the fallback re-registered the SessionStart hook
and its install-time trust prompt on Codex.

Removing the Codex hook file and the manifest `hooks` pointer (commit
"Remove Codex hooks") did not disable the hook on Codex — it removed the
explicit declaration that was overriding the fallback, so the fallback
took over and found the Claude hooks/hooks.json.

Declare an empty inline hooks object ({}) in .codex-plugin/plugin.json.
It parses as an empty inline hook set and stops Codex reaching the
auto-discovery fallback. An absent field, an empty array ([]), and an
empty inline list all collapse back to the fallback, so the value must
be exactly {}.

Update the test to assert the manifest declares hooks: {} (and that
hooks/hooks.json exists, which is what makes the declaration necessary),
replacing the prior assertion that the field was absent — which passed
while the hook was still being auto-discovered.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… docs

hooks/session-start-codex has had no caller since "Remove Codex hooks"
(obra#1845) deleted hooks-codex.json and its manifest registration; the
Codex manifest now declares an empty hooks object so Codex registers no
session-start hook at all. The script is Codex-specific dead code —
nothing executes it on Codex or any other harness.

- Delete hooks/session-start-codex.
- tests/hooks/test-session-start.sh: drop the two Codex cases that are
  redundant with the generic session-start tests (nested-format and the
  legacy-warning omission are already covered by the Claude Code cases).
  Re-point the "wrapper dispatches" case to the live `session-start`
  script so run-hook.cmd dispatch coverage — used by Claude Code and
  Cursor in production — is preserved rather than lost.
- docs/porting-to-a-new-harness.md: Codex is no longer a Shape A
  (shell-hook) harness, so re-anchor that worked example to Cursor (a
  live shell-hook harness that demonstrates the same per-harness field,
  schema, and matcher variance) and mark Codex as native skill discovery
  with no session-start hook. Clears the references to the deleted
  hooks-codex.json.
- docs/windows/polyglot-hooks.md: the "check hooks-codex.json" pointer
  referenced a file deleted in obra#1845; re-point to hooks-cursor.json.

RELEASE-NOTES.md keeps its historical mention of hooks-codex.json (it
accurately records what that release did). The tests/codex-plugin-sync
fixtures build their own synthetic session-start-codex and test the sync
mechanism generically, so they are intentionally left as-is.

Co-Authored-By: Claude Opus 4.8 (1M context) <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.

3 participants