charles/agent-hooks

Fork 0

fix(flows): render address-review skill before dispatching author #1221

Merged

charles merged 2 commits from fix/render-skill-address-review into main

2026-05-15 21:10:06 +00:00

claude-desktop commented

2026-05-15 20:47:01 +00:00

Collaborator

Summary

review-changes-requested.yml was dispatching with task: event.pr.title (bare PR title) — agent received no instruction to read review comments or invoke the address-review skill; it would re-implement from scratch instead of fixing feedback
Add render_skill op + RenderSkillCapability that calls renderPrompt against the agent's type and skill name; flow now renders the address-review template before dispatch
Dispatch gates on steps.skill.outputs.found so missing skill aborts cleanly rather than sending an empty prompt
Also commits the in-progress nav-sections.tsx sessionStorage-based back navigation (was breaking the pre-push QA gate) with the stale test updated to match

Commits

fix(flows): render address-review skill before dispatching author

New op render_skill (+ test) wrapping RenderSkillCapability.render
RenderSkillCapability / RenderSkillAdapterDeps added to types.ts + live-capabilities.ts
Live wiring in engine-bootstrap.ts: resolves agent by login → calls renderPrompt with filesystemFallback: true
Shadow capability passes through to live dep (read-only, no side effects to suppress)
ops/index.ts registry + registry.test.ts count bump (31 → 32) + representative output
4 existing test fixtures updated with renderSkill: { render: async () => ({ body: null }) } stub
flows.schema.json regenerated (33 ops)

fix(web): sessionStorage-based back navigation + update stale test

BackToMainNav in nav-sections.tsx: replace window.history.back() with sessionStorage lookup (ch_pre_settings_path), useEffect tracks last non-settings path
sidebar-nav.test.tsx: stale assertion (expect(back).toHaveBeenCalledTimes(1)) flipped to expect(back).not.toHaveBeenCalled()

Test plan

just qa green (3367 server tests pass, web tests pass with chromium)
Trigger pull_request_review_rejected webhook on a dev-authored PR → confirm agent receives rendered address-review body (not bare PR title) in task log
Trigger same webhook when address-review skill row missing → confirm dispatch is skipped (no empty-body task enqueued)
Navigate to /settings/appearance, then back → confirm landing on last non-settings path

🤖 Generated with Claude Code

## Summary - `review-changes-requested.yml` was dispatching with `task: event.pr.title` (bare PR title) — agent received no instruction to read review comments or invoke the `address-review` skill; it would re-implement from scratch instead of fixing feedback - Add `render_skill` op + `RenderSkillCapability` that calls `renderPrompt` against the agent's type and skill name; flow now renders the `address-review` template before dispatch - Dispatch gates on `steps.skill.outputs.found` so missing skill aborts cleanly rather than sending an empty prompt - Also commits the in-progress `nav-sections.tsx` sessionStorage-based back navigation (was breaking the pre-push QA gate) with the stale test updated to match ## Commits **`fix(flows): render address-review skill before dispatching author`** - New op `render_skill` (+ test) wrapping `RenderSkillCapability.render` - `RenderSkillCapability` / `RenderSkillAdapterDeps` added to `types.ts` + `live-capabilities.ts` - Live wiring in `engine-bootstrap.ts`: resolves agent by login → calls `renderPrompt` with `filesystemFallback: true` - Shadow capability passes through to live dep (read-only, no side effects to suppress) - `ops/index.ts` registry + `registry.test.ts` count bump (31 → 32) + representative output - 4 existing test fixtures updated with `renderSkill: { render: async () => ({ body: null }) }` stub - `flows.schema.json` regenerated (33 ops) **`fix(web): sessionStorage-based back navigation + update stale test`** - `BackToMainNav` in `nav-sections.tsx`: replace `window.history.back()` with sessionStorage lookup (`ch_pre_settings_path`), `useEffect` tracks last non-settings path - `sidebar-nav.test.tsx`: stale assertion (`expect(back).toHaveBeenCalledTimes(1)`) flipped to `expect(back).not.toHaveBeenCalled()` ## Test plan - [ ] `just qa` green (3367 server tests pass, web tests pass with chromium) - [ ] Trigger `pull_request_review_rejected` webhook on a dev-authored PR → confirm agent receives rendered `address-review` body (not bare PR title) in task log - [ ] Trigger same webhook when `address-review` skill row missing → confirm dispatch is skipped (no empty-body task enqueued) - [ ] Navigate to `/settings/appearance`, then back → confirm landing on last non-settings path 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-desktop added 2 commits

2026-05-15 20:47:02 +00:00

fix(flows): render address-review skill before dispatching author 20eb55dc4e

The review-changes-requested.yml flow was dispatching with
task: event.pr.title (bare PR title) — the agent received no
instruction to read review comments or invoke the address-review
skill. It would re-implement from scratch instead of fixing feedback.

Add render_skill op (RenderSkillCapability) that calls renderPrompt
against the author's agent type + skill name. The flow now renders
the address-review template ({{pr_number}}, {{title}} interpolated)
before dispatch. Falls back gracefully when skill is missing (found:
false gate skips dispatch).

Wire render_skill into live, shadow, and test capability bags.
Regenerate flows.schema.json (33 ops → 32+1).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(web): sessionStorage-based back navigation + update stale test

qa / i18n-string-check (pull_request) Successful in 14s

Details

qa / db-schema (pull_request) Successful in 19s

Details

qa / sql-layer-check (pull_request) Successful in 24s

Details

qa / dockerfile (pull_request) Successful in 28s

Details

qa / qa-1 (pull_request) Successful in 4m9s

Details

qa / qa (pull_request) Successful in 0s

Details

bc0c462363

Replace window.history.back() in BackToMainNav with sessionStorage
lookup: remember the last non-settings path via useEffect and navigate
back to it on click (falling back to /board). Sidebar test updated
to assert history.back is NOT called (new behaviour), not that it is.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

charles requested review from reviewer

2026-05-15 20:52:18 +00:00

reviewer approved these changes

2026-05-15 20:54:18 +00:00

reviewer left a comment

Review — `fix(flows): render address-review skill before dispatching author`

Overview

Correctly fixes a real bug: review-changes-requested.yml was dispatching task: event.pr.title (bare PR title) to the author agent, giving it no signal to read review comments or invoke the address-review skill. The new render_skill op renders the correct template before dispatch and gates on steps.skill.outputs.found, cleanly aborting when the skill is missing.

✅ What's done well

Root cause fixed correctly. The dispatch step now receives the fully-rendered address-review body. The found guard prevents empty-body task enqueue.
Clean capability abstraction. RenderSkillCapability (object-input) and RenderSkillAdapterDeps (positional) follow the established cap/adapter pattern exactly. Shadow pass-through is correctly marked read-only.
Test coverage solid. render_skill.test.ts covers arg validation, found=true, found=false (null), empty-vars default, and outputsSchema round-trip. The 4 existing test fixtures get the correct { body: null } stub.
Schema regenerated in both locations — no drift between server and web public schemas.
filesystemFallback: true is the right call for resilient cold-start skill resolution.
Flow YAML is correct. The render_skill step inherits all preconditions from the old dispatch; dispatch additionally gates on steps.skill.outputs.found. Vars passed (repo, pr_number, title) are appropriate for the address-review template.
Nav fix is clean. Replaces fragile window.history.length > 1 heuristic with deterministic sessionStorage lookup; falls back to /board.

⚠️ Minor issues

1. found derived solely from body !== null — empty string passes silently

// render_skill.ts
found: result.body !== null,  // "" would be found: true

If renderPrompt ever returns "" for a malformed/empty skill file, found is true and dispatch fires with an empty task. If renderPrompt has a documented null-for-empty contract this is a non-issue; otherwise a one-liner result.body !== null && result.body.length > 0 closes it.

2. Type cast in render_skill.ts is slightly unsafe

const deps = ctx.deps as { renderSkill: RenderSkillCapability };

A misconfigured capability bag produces TypeError: Cannot read properties of undefined rather than a clear "missing dep" error. Consistent with the existing op pattern — acceptable here, systemic issue to track separately.

3. getSettings() called per render invocation — trivial given once-per-webhook usage, low priority.

4. Bundled unrelated web fix — explained in the PR description (QA gate blocker), acknowledged.

Verdict

APPROVED. The fix is correct, well-implemented, and follows project conventions throughout. The empty-string edge case is the only substantive concern — worth a one-line guard if renderPrompt doesn't already guarantee null-for-empty output.

## Review — `fix(flows): render address-review skill before dispatching author` ### Overview Correctly fixes a real bug: `review-changes-requested.yml` was dispatching `task: event.pr.title` (bare PR title) to the author agent, giving it no signal to read review comments or invoke the `address-review` skill. The new `render_skill` op renders the correct template before dispatch and gates on `steps.skill.outputs.found`, cleanly aborting when the skill is missing. --- ### ✅ What's done well - **Root cause fixed correctly.** The dispatch step now receives the fully-rendered `address-review` body. The `found` guard prevents empty-body task enqueue. - **Clean capability abstraction.** `RenderSkillCapability` (object-input) and `RenderSkillAdapterDeps` (positional) follow the established cap/adapter pattern exactly. Shadow pass-through is correctly marked read-only. - **Test coverage solid.** `render_skill.test.ts` covers arg validation, found=true, found=false (null), empty-vars default, and outputsSchema round-trip. The 4 existing test fixtures get the correct `{ body: null }` stub. - **Schema regenerated in both locations** — no drift between server and web public schemas. - **`filesystemFallback: true`** is the right call for resilient cold-start skill resolution. - **Flow YAML is correct.** The `render_skill` step inherits all preconditions from the old dispatch; dispatch additionally gates on `steps.skill.outputs.found`. Vars passed (`repo`, `pr_number`, `title`) are appropriate for the `address-review` template. - **Nav fix is clean.** Replaces fragile `window.history.length > 1` heuristic with deterministic sessionStorage lookup; falls back to `/board`. --- ### ⚠️ Minor issues **1. `found` derived solely from `body !== null` — empty string passes silently** ```ts // render_skill.ts found: result.body !== null, // "" would be found: true ``` If `renderPrompt` ever returns `""` for a malformed/empty skill file, `found` is `true` and dispatch fires with an empty task. If `renderPrompt` has a documented null-for-empty contract this is a non-issue; otherwise a one-liner `result.body !== null && result.body.length > 0` closes it. **2. Type cast in `render_skill.ts` is slightly unsafe** ```ts const deps = ctx.deps as { renderSkill: RenderSkillCapability }; ``` A misconfigured capability bag produces `TypeError: Cannot read properties of undefined` rather than a clear "missing dep" error. Consistent with the existing op pattern — acceptable here, systemic issue to track separately. **3. `getSettings()` called per render invocation** — trivial given once-per-webhook usage, low priority. **4. Bundled unrelated web fix** — explained in the PR description (QA gate blocker), acknowledged. --- ### Verdict **APPROVED.** The fix is correct, well-implemented, and follows project conventions throughout. The empty-string edge case is the only substantive concern — worth a one-line guard if `renderPrompt` doesn't already guarantee null-for-empty output.

charles merged commit bf573738dd into main

2026-05-15 21:10:06 +00:00

charles deleted branch fix/render-skill-address-review

2026-05-15 21:10:07 +00:00

charles referenced this pull request from a commit

2026-05-15 21:10:07 +00:00

fix(flows): render address-review skill before dispatching author (#1221)