refactor(engine): remove dead workflow lifecycle hooks (#476)

[Rounak Kumar Agarwal (RounakKumarAgarwal)]

Closes #476.

What

The hooks: block (on_start / on_complete / on_error) was documented and parsed, but the feature was dead code: hook templates were rendered and the result discarded. Nothing observed it — no event, no log entry, no dashboard/console output, no context write, no side effect — so a hook was completely unobservable, including when it failed.

Changes

• Schema — removed HooksConfig and WorkflowDef.hooks from config/schema.py, and the re-exports from config/__init__.py.
• Engine — removed _execute_hook, LifecycleHookResult, and all call sites from engine/workflow.py, plus now-stale docstring references.
• Docs — removed the Hooks section (and TOC entry / schema-overview snippet) from docs/workflow-syntax.md.
• Tests — removed the hook-specific tests and fixture (tests/fixtures/valid_full.yaml); added TestHooksRemoved asserting a workflow declaring hooks: now fails validation with a clear error.
• CHANGELOG — noted the removal.

Acceptance criteria

• hooks: removed from the schema — declaring it produces a clear validation error (extra_forbidden).
• _execute_hook / LifecycleHookResult and all call sites removed from the engine.
• Hooks section removed from docs/workflow-syntax.md.
• CHANGELOG entry added.

Notes

If lifecycle hooks are wanted later, the right syntax should be designed against a concrete requirement (emitting an event, invoking a type: script step, or calling a webhook) rather than restoring a placeholder that renders a template and throws it away.

Because hooks: was documented and previously parsed (silently doing nothing), this is technically a breaking change for any workflow that declares it — those will now fail validation, which is the intended behavior per the issue.

Verified locally: ruff check/format clean, affected test suites green. The remaining ty diagnostics are pre-existing Windows-only platform issues (termios/signal.SIGKILL/etc.) present on a clean checkout, unrelated to this change.

[Rounak Kumar Agarwal (RounakKumarAgarwal)]

@microsoft-github-policy-service agree

[Jason Robert (jrob5756)]

Two blocking issues here, both real: a mangled comment that undoes the readability this refactor was supposed to deliver, and a bundled skill that still teaches agents to write a hooks: block that now hard-fails validation. Once those are fixed this is fine to merge — the three recommended items are cleanup, not blockers.

Blocking:

• src/conductor/engine/workflow.py:5343 — duplicated lines and a broken sentence in the except BaseException comment (see inline).
• No single line — the conductor skill (SKILL.md, authoring.md, yaml-schema.md) still documents hooks:, and a workflow using it now fails to load instead of just silently no-opping like before this PR.

b2 — BLOCKING: the bundled conductor skill still teaches agents to write hooks:, which now hard-fails validation

docs/workflow-syntax.md got cleaned up, but the skill that ships with the wheel didn't. plugins/conductor/skills/conductor/skills/conductor/SKILL.md:111, references/authoring.md:51-54, and references/yaml-schema.md:133-138 all still document the removed hooks: block — and this is the file skills/registry.py loads whenever a workflow declares skills: [conductor].

This is worse than typical doc drift because of how the removal was implemented. WorkflowDef uses extra="forbid", so hooks: doesn't degrade to a no-op anymore — it aborts the load. I confirmed it: a workflow with workflow.hooks: fails conductor validate with exit code 1 and workflow.hooks: Extra inputs are not permitted. So an agent following this skill will generate a workflow that can't run at all. Before this PR, the same generated YAML at least executed.

I checked that these are genuinely the removed workflow-level feature and not the unrelated plugin hooks/ directories (plugins/manifest.py, config/validator.py, README.md:229, examples/plugins.yaml:39 are correctly untouched) — these three references are specifically workflow:-scoped hooks: keys with on_start/on_complete/on_error children.

Worth flagging as a process gap too: docs/workflow-syntax.md and these skill references are near-duplicate YAML references, and only the first got updated here. That's exactly how this slipped through, and it'll happen again on the next syntax change unless there's a check for it. A grep -rn 'hooks:' plugins/ docs/ examples/ before merge would have caught it.

Suggested fix: Delete the table row at SKILL.md:111; delete lines 51-54 of authoring.md (and the blank line that's now orphaned); delete lines 133-138 of yaml-schema.md including the # Lifecycle hooks comment, leaving one blank line between the cost: block and the # Arbitrary metadata for downstream tooling comment.

r1 — RECOMMENDED: stale "on_error hooks" prose survives in three places the sweep missed

The parallel wording got scrubbed correctly at engine/workflow.py:2109, engine/workflow.py:2136, and docs/workflow-syntax.md:1158, but three mirrors of the same sentences didn't:

• src/conductor/exceptions.py:664 — the SubworkflowTerminatedError docstring still says the attributes are carried "...so on_error hooks, debugging surfaces, and the CLI can inspect it without walking __cause__." This one's worth more attention than a normal stale comment, since it's the stated reason terminated_output/terminated_reason/terminated_by live on the exception at all. Point at a consumer that no longer exists and someone will eventually read this as dead code and strip real diagnostic payload off a sub-workflow failure.
• AGENTS.md:216 — the Terminate steps bullet still says "; runs on_complete hook" and ", runs on_error hook." This file gets loaded into agent context on basically every task in this repo, so the stale claim has wide reach.
• plugins/conductor/skills/conductor/references/authoring.md:657, 658, 660 — "Runs the on_complete hook.", "Runs the on_error hook.", and "...for on_error hooks and debugging surfaces." Line 660 is the exact sentence already fixed at docs/workflow-syntax.md:1158 — just not here.

All three describe behavior that doesn't happen anymore.

Suggested fix: Match the wording already used in docs/workflow-syntax.md:1158. In exceptions.py:664: "the wrapper so debugging surfaces and the CLI can inspect it without walking __cause__." In AGENTS.md:216: drop "; runs on_complete hook" and ", runs on_error hook". In authoring.md: drop the "Runs the ... hook." sentences at 657 and 658, and on 660 change "for on_error hooks and debugging surfaces" to "for debugging surfaces".

[Codecov Comments Bot (codecov-commenter)]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@3c07139). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #480   +/-   ##
=======================================
  Coverage        ?   91.77%           
=======================================
  Files           ?      146           
  Lines           ?    23844           
  Branches        ?        0           
=======================================
  Hits            ?    21882           
  Misses          ?     1962           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Rounak Kumar Agarwal (RounakKumarAgarwal)]

Thanks for the thorough review — all five items addressed in 73e3b9d.

Blocking:

B1 (garbled except BaseException comment) — restored the comment using your suggested text, including the is_base_exception paragraph documenting what the flag is for. The duplicated lines and cut-off clause are gone; no executable lines were touched.
B2 (bundled skill still teaches hooks:) — removed the hooks: block from all three skill files: the table row in SKILL.md, the YAML block in references/authoring.md, and the block in references/yaml-schema.md. An agent following the skill will no longer generate a workflow that hard-fails.

Recommended:

R1 (stale "on_error hooks" prose) — reworded exceptions.py (the SubworkflowTerminatedError docstring), AGENTS.md:216, and authoring.md:657-660 to match the wording already used in docs/workflow-syntax.md. I also found and fixed one more the sweep missed: a docstring in tests/test_engine/test_subworkflow.py.
R2 (error message oversells) — went with the validator approach: added a _reject_removed_fields model validator on WorkflowDef that raises a targeted message naming #476 and pointing at the workflow_completed/workflow_failed events. Reworked the tests accordingly — dropped the brittle extra_forbidden assertion, and now assert the removal message at three layers: schema constructor (test_schema.py), YAML loader (test_loader.py), and CLI conductor validate (test_validate.py, with a new invalid_hooks.yaml fixture).
R3 (CHANGELOG spacing) — fixed the double blank line after ## [Unreleased] and the missing blank line before ### Added. Left the ordering as-is since, as you noted, the file is already inconsistent.

On the process gap: agreed a grep -rn 'hooks:' plugins/ docs/ examples/ would've caught B2. I ran that sweep this round and it's clean apart from the intended references (the validator message and the CHANGELOG entry).

[Rounak Kumar Agarwal (RounakKumarAgarwal)]

CI note: the one failing check (Test (Python 3.13, ubuntu-latest)) is a flaky Fleet TUI test, not a regression from this PR.

It failed on tests/test_fleet/test_tui_runs.py::TestRunsScreenPausesWhileNotOnTop::test_the_animation_timer_is_paused_and_resumed — a timing assertion on the splash animation timer. An earlier run of this same branch failed instead on a different set of Fleet TUI tests (test_tui_theme.py::TestShortenHome and test_tui_actions.py::TestDirectoryPickerModal, on the Windows job). Different platform, different tests, all under tests/test_fleet/ — the inconsistency across runs points to pre-existing flakiness/platform-fragility in the Fleet TUI suite rather than anything this change introduced.

For reference, this PR's changes (git diff --name-only upstream/main...HEAD) don't touch fleet/ or tui/ at all — they're limited to config/schema, engine, exceptions, the bundled conductor skill docs, and the corresponding tests. Every test this PR adds or modifies passes on all jobs. A re-run of the failed job should likely go green; happy to open a separate issue for the flaky Fleet tests if that's useful.

[Jason Robert (jrob5756)]

LGTM, thanks for contributing!

[Rounak Kumar Agarwal (RounakKumarAgarwal)]

LGTM, thanks for contributing!

Thank you! I’m really interested in contributing more to the project. How can I connect with you to collaborate on issues and work on such repositories? I’d love to learn from the community and contribute more. Looking forward to hearing from you!