Skip to content

refactor(agents): remove preExecute hook - move postInitialPRComment to ack infrastructure#607

Merged
zbigniewsobiecki merged 2 commits intodevfrom
feature/remove-pre-execute-hook
Mar 2, 2026
Merged

refactor(agents): remove preExecute hook - move postInitialPRComment to ack infrastructure#607
zbigniewsobiecki merged 2 commits intodevfrom
feature/remove-pre-execute-hook

Conversation

@aaight
Copy link
Copy Markdown
Collaborator

@aaight aaight commented Mar 2, 2026

Summary

Removes the 'Post-Execute Hook' feature (preExecute: postInitialPRComment) from the agent definition system. The initial PR comment posting is now handled entirely by the acknowledgment infrastructure in src/triggers/github/ack-comments.ts, which already posts acknowledgment comments before agent execution begins.

  • Remove preExecute: postInitialPRComment from review.yaml and respond-to-ci.yaml
  • Remove preExecute field from BackendSchema in schema.ts
  • Remove PRE_EXECUTE_REGISTRY from strategies.ts and its postInitialPRCommentHook import
  • Remove postInitialPRCommentHook function and PreExecuteParams interface from contextSteps.ts
  • Remove preExecute?() from AgentProfile interface in profiles.ts and the wiring code
  • Remove the if (profile.preExecute) block from adapter.ts
  • Remove dead exports from index.ts
  • Update tests to remove assertions about the removed feature

Trello card: https://trello.com/c/69a5ae8cfbe59409cccdef08

Test plan

  • All 3731 tests pass
  • TypeScript type checking passes
  • Lint passes (Biome)
  • The existing ack infrastructure in src/triggers/github/ack-comments.ts already handles posting initial PR comments for all GitHub PR triggers via maybePostAckComment in the webhook handler

🤖 Generated with Claude Code

@aaight
Copy link
Copy Markdown
Collaborator Author

aaight commented Mar 2, 2026

CI Failures Resolved

Root Cause

The lint-and-test check failed during the Build frontend step with TypeScript errors:

  • agent-definition-editor.tsx(460,26): Property preExecute does not exist on the backend schema type
  • agent-definition-editor.tsx(461,40): Argument "preExecute" is not assignable to the backend schema keys

Two locations still referenced the removed preExecute property that weren't caught in the original PR changes.

Fixes Applied

  1. tests/unit/agents/definitions/loader.test.ts — Removed the expect(def.backend.preExecute).toBeUndefined() assertion from the implementation agent roundtrip test (property no longer exists on the type)
  2. web/src/components/settings/agent-definition-editor.tsx — Removed the Pre-Execute Hook select dropdown UI section and changed the remaining Post-Configure Hook from a 2-column grid to a single-column layout

Verification

  • ✅ Backend build passes
  • ✅ Frontend build passes (tsc -b && vite build)
  • ✅ TypeScript typecheck passes (tsc --noEmit)
  • ✅ Biome lint passes (795 files checked)
  • ✅ All 218 test files, 3731 tests pass
  • ✅ Changes pushed to branch

…d editor

The loader test still had an assertion checking def.backend.preExecute,
and the agent definition editor still rendered a Pre-Execute Hook select
dropdown, both referencing the removed preExecute schema property.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@nhopeatall
Copy link
Copy Markdown
Collaborator

nhopeatall commented Mar 2, 2026

✨ On it — checking the PR for feature/remove-pre-execute-hook


Progress: [░░░░░░░░░░] 4% (iteration 3/70)

🔍 Code Review Update (1 min)

I've begun my review of the pull request, which focuses on refactoring the preExecute hook infrastructure. I've started by examining the PR diff to understand the changes and the rationale behind them, specifically noting the claim that ack comment posting is now handled elsewhere. My next step is to verify this claim and ensure the removal of preExecute doesn't introduce regressions.

Last updated: iteration 3 · review

Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

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

Summary

Clean, thorough removal of the preExecute hook infrastructure across all layers — schema, registries, profiles, adapter, YAML configs, UI, and tests. The change is safe and well-scoped.

Notes

  • Dependency analysis confirms postInitialPRCommentHook had exactly one consumer (PRE_EXECUTE_REGISTRY), so removal is complete with no dangling references.
  • The strategies.ts docstring was correctly updated ("Contains registries for context pipeline steps."), but the contextSteps.ts module docstring on line 2 still reads "Context pipeline step implementations and pre-execute hooks." — worth updating to match.
  • UI correctly adjusts from grid-cols-2 to grid-cols-1 since only "Post-Configure Hook" remains.
  • All CI checks pass.

Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

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

LGTM - Clean, thorough removal of preExecute hook infrastructure. One minor note: contextSteps.ts line 2 docstring still mentions 'pre-execute hooks' and should be updated.

@zbigniewsobiecki zbigniewsobiecki merged commit 6db4121 into dev Mar 2, 2026
6 checks passed
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