You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Run go get github.com/snyk/go-application-framework@YOUR_LATEST_GAF_COMMIT in the cliv2 directory.
Tip: for local testing, you can uncomment the line near the bottom of the CLI's go.mod to point to your local GAF code.
Run go mod tidy in the cliv2 directory.
Run the CLI tests and do any required manual testing.
Open a PR in the CLI repo now with the go.mod and go.sum changes.
Once this PR is merged, repeat these steps, but pointing to the latest GAF commit on main and update your CLI PR.
Note
Medium Risk
Touches the central Invoke path (panic recovery, hook goroutines, timeout) so regressions could affect every workflow run; behavior is heavily tested and hooks are opt-in registration.
Overview
Adds post-invoke hooks that run after each top-levelEngine.Invoke completes, including when the workflow is missing or the callback returns an error.
Hooks receive an InvokeOutput (workflow id, data, error) and a scoped Engine. Registration is via AddPostInvokeHook / PostInvokeHookRegistrar, with workflow.AddPostInvokeHook and app WithPostInvokeHooks. Hooks must be registered before Init; late registration errors.
Nested invokes (sub-workflows via engineWrapper / invocation.GetEngine()) do not fire hooks; the wrapper injects withNested(). Hooks that invoke workflows through the engine argument stay nested and avoid recursion.
Hooks run concurrently with a shared 5s timeout context; slow hooks are logged and Invoke does not wait indefinitely. Hook and callback panics are recovered so other hooks still run and panics surface as errors to hooks before the original panic is re-raised.
initialized on EngineImpl is now atomic.Bool. Mocks add InvokeOutput and PostInvokeHookRegistrar.
Reviewed by Cursor Bugbot for commit 2c1f800. Bugbot is set up for automated code reviews on this repo. Configure here.
PR Type
Enhancement, Bug fix
Description
Introduce post-invoke hooks for engine invocations.
Hooks run concurrently after top-level invokes.
Skip hooks for nested invocations.
Fix hook registration race condition.
Add configurable timeout for hooks.
Diagram Walkthrough
flowchart LR
A[Engine.Invoke] --> B{Workflow Found};
B -- Yes --> C[Workflow Callback];
B -- No --> D[Invoke Error];
C --> E(Post Invoke Hooks);
D --> E;
E -- Concurrent, Timed --> F[Hook Completion];
A -- Nested Invoke --> G[Engine.Invoke (Nested)];
G --> C;
G -- Hooks Skipped --> F;
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Automated verification (/verification) — re-run on new commit
Approval policy: this automation auto-approves only when a PR is a pure dependency bump (go.mod/go.sum only, no other files touched). This PR modifies pkg/app/options.go, pkg/app/app_test.go, pkg/mocks/workflow.go, and pkg/workflow/*.go — core engine code, no dependency-manifest changes — so it does not qualify and is not auto-approved.
Note on this automation's prior reviews: this automation previously left two CHANGES_REQUESTED reviews on this PR. Per updated policy, this automation will no longer request changes — findings are reported as comments only, and the PR is never blocked by this automation. I attempted to formally dismiss those prior reviews, but the available tooling can only dismiss prior approvals, not CHANGES_REQUESTED reviews — that requires a maintainer to click "Dismiss review" on GitHub. The concerns those reviews raised have been re-triaged below against the current commit; several are now confirmed fixed.
Ran all four verification passes (semantic analysis, adversarial review, security scan, independent code review) against the full diff (base 0980355 → head 258292b). CI is green on this exact SHA (build/lint/unit-with-race/Windows/security-scans/Snyk code+license+secrets all pass) and is treated as the build/test oracle.
Since the last review (base 0980355 → previous head 3e6fbe7)
✅ Fixed: hook recursion guard — hooks now receive a nested-tagged engineWrapper, not the raw *EngineImpl, and the hook loop is wrapped in recover(). Verified against Test_EngineWrapper_HookRecursionGuard / Test_PostInvokeHook_PanicRecovery.
✅ Fixed: GetWorkflowID() renamed to GetWorkflowIdentifier(), now consistent with InvocationContext.
✅ Fixed: WithPostInvokeHooks now has a dedicated test (Test_WithPostInvokeHooks in app_test.go).
❌ Still open: unmarked-as-breaking addition to the exported Engine interface — non-blocking, matches existing precedent (GetRuntimeInfo/SetRuntimeInfo) and GAF stays on v0.
❌ Still open: hooks are silently skipped when Invoke is called before Init() — low real-world impact, informational only.
Should Fix (new this round)
Unsynchronized read of e.logger in the new panic-recovery handler (pkg/workflow/engineimpl.go:398) — independently found by three of four reviewer passes. Every other access to logger in this file goes through e.mu (GetLogger/SetLogger), but the new recover() handler reads e.logger directly with no lock held. A concurrent SetLogger call while a hook panics is a genuine data race (go test -race would flag it if a test exercised the combination — none currently do). Fix: use e.GetLogger() instead of the raw field.
Recursion guard is a convention on the eng parameter, not a structural invariant of Invoke (pkg/workflow/engineimpl.go:393, doc comment at pkg/workflow/types.go) — flagged independently by two adversarial-review lenses (Skeptic and Architect). The nested-guard only applies when hook code calls Invoke on the eng parameter it's handed. Any hook closure that instead calls Invoke on the raw engine reference it already holds (the same instance used to register the hook via AddPostInvokeHook/WithPostInvokeHooks) bypasses withNested() entirely and re-fires all hooks with no cycle guard — recursing until the goroutine's stack overflows. Critically, a stack-overflow crash is not caught by the recover() added for ordinary panics. This repo already has the exact anti-pattern this depends on (pkg/devtools/tree.go invokes the raw engine directly rather than through invocation.GetEngine()), so it's a realistic misuse path, not a hypothetical. Fix direction: either make the nested-tracking structural (e.g. a context value checked at the top of EngineImpl.Invoke regardless of which Engine reference is used), or strengthen the doc comment with an explicit warning against invoking through any reference other than the eng parameter passed into the hook.
hookEngine doesn't inherit context/instrumentation defaults, unlike the wrapper built for workflow callbacks (pkg/workflow/engineimpl.go:393) — flagged both by the adversarial review's Architect lens and independently by Bugbot's own review on this commit ("Hook engine lacks invoke defaults"). Contrast with localEngine (built a few lines earlier for invocation.GetEngine()), which sets defaultInstrumentationCollector and defaultCtxFunc so nested calls automatically inherit the parent's context/collector. hookEngine := &engineWrapper{WrappedEngine: e} sets neither, so a hook that calls eng.Invoke(id) without explicitly passing WithContext(ctx) silently loses cancellation/trace propagation that an equivalent workflow-side nested call would get for free. Fix: hookEngine := &engineWrapper{WrappedEngine: e, defaultCtxFunc: func() context.Context { return hookCtx }}.
Suggestions (non-blocking)
PostInvokeHook's doc comment ("called ... after the workflow callback returns") is inaccurate for the missing-workflow case — Test_PostInvokeHook_FiresForMissingWorkflow confirms hooks also fire when no callback ever ran. Worth an explicit note to avoid consumers misreading GetError() as always callback-sourced.
No test covers a nested invoke of a nonexistent workflow (the opts loop added to the !ok branch specifically exists to keep that case correctly tagged nested — currently unverified by any test).
No test covers a hook observing values/deadlines propagated via WithContext.
No test exercises a real race between AddPostInvokeHook and concurrent Invoke calls (current concurrency test registers the hook before any goroutine starts).
Minor: addPostInvokeHook test helper and two tests in enginewrapper_test.go do an unnecessary e.(*EngineImpl) type assertion just to call AddPostInvokeHook, which is now part of the Engine interface — the assertion can be dropped.
Security
No exploitable findings. No dependency-manifest changes to scan. Manual trace of the hook mechanism found no attacker-reachable surface (hooks are registered only by trusted host code, not workflow extensions); CI's Snyk code/license/secrets checks are all green.
What's solid
The AddPostInvokeHook/Invoke locking discipline is correctly serialized (write under Lock(), read/copy under RLock(), hooks run outside the lock), nested-invocation tagging propagates correctly through the well-behaved call path, and per-hook panic recovery correctly lets subsequent hooks continue. Test coverage of the intended behavior (ordering, error propagation, missing-workflow, concurrent top-level invocations, nested-skip, nil-hook, panic-recovery) is thorough.
None of the above findings are release-blocking crashes in the tested/documented usage path — they're hardening gaps and a data race in an edge case. Happy to re-review on the next push.
Sent by Cursor Automation: Automatic PR verification
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Automated verification (/verification) — re-run on new commit
Approval policy: this automation auto-approves only when a PR is a pure dependency bump (go.mod/go.sum only, no other files touched). This PR's diff (base 0980355 → head 6fc3dcf) touches pkg/app/options.go, pkg/app/app_test.go, pkg/mocks/workflow.go, and pkg/workflow/*.go — core engine code — with no go.mod/go.sum changes at all. It does not qualify, so it is not auto-approved.
CI is green on this exact SHA (build, lint, unit tests with race detector, Windows tests, security-scans, and Snyk code/license/secrets checks all pass), and is treated as the build/test oracle for this run.
Since the previous review (head 258292b → head 6fc3dcf)
✅ Fixed: unsynchronized read of e.logger in the panic-recovery handler — now goes through e.GetLogger() like every other access in this file.
✅ Fixed: hookEngine now carries defaultCtxFunc: func() context.Context { return hookCtx }, so a hook's nested Invoke calls inherit the top-level context's values/deadline/cancellation instead of silently losing them. New test Test_PostInvokeHook_ReceivesContextValues covers this.
✅ Addressed: the structural-vs-convention recursion guard concern was resolved via the documented alternative fix direction — PostInvokeHook's doc comment now carries an explicit WARNING that hooks must invoke through the provided engine parameter, not a captured raw engine reference, or the guard is bypassed.
✅ Fixed: doc comment now explicitly states hooks fire "including when the workflow is not found," clearing up the earlier ambiguity around GetError() not always being callback-sourced. New test Test_PostInvokeHook_NestedInvokeOfMissingWorkflow also covers a nested invoke of a missing workflow correctly staying tagged as nested.
Still open (non-blocking, informational)
Adding AddPostInvokeHook to the exported Engine interface is a breaking-ish addition for any out-of-repo implementer of Engine — matches existing precedent (GetRuntimeInfo/SetRuntimeInfo) and GAF intentionally stays on major version 0, so not a novel violation.
Post-invoke hooks are still silently skipped if Invoke is called before Init() (early-return path predates the hook-firing block). Low real-world impact since production engines are always Init()'d first.
Minor: unlike defaultCtxFunc (now fixed above), hookEngine still doesn't set defaultInstrumentationCollector, so a hook-initiated nested Invoke won't inherit the top-level instrumentation collector the way a workflow-side nested call would.
Suggestion (unchanged): PostInvokeContext is modeled as an interface with a single implementation and an unused generated mock — a plain exported struct would cover current callers with less indirection.
Security
No exploitable findings. No dependency-manifest changes to scan. Hooks are registered only by trusted host code, not workflow extensions, so no new attacker-reachable surface.
Note on this automation's prior reviews
This automation previously left two CHANGES_REQUESTED reviews on this PR. Per current policy, this automation will not request changes on any future run — findings are reported as comments only, and this PR is never blocked by this automation. The available tooling can only dismiss this automation's own prior approvals, not CHANGES_REQUESTED reviews, so those two reviews remain visible on the PR; a maintainer with write access would need to dismiss them manually from the GitHub UI if a clean review state is desired.
None of the remaining findings are release-blocking in the tested/documented usage path. Happy to re-review on the next push.
Sent by Cursor Automation: Automatic PR verification
The reason will be displayed to describe this comment to others. Learn more.
question: So hookCtx has the timeout, but hookEngine defaults nested invocations to the original ctx. In this case, doesn't eng.Invoke(...) continue after the hook timeouts? Is that intentional, or should hookEngine also use hookCtx?
The per-subtest isolation loop cleared only the six proxy variables, but
the test also asserts on NODE_EXTRA_CA_CERTS, KRB5_CONFIG and KRB5CCNAME.
Those three were read from the ambient environment, so every subtest
expecting them empty failed on any machine that sets them.
Clear all nine variables DetectProxyConfig reads, keeping the list in
step with envVarSpecs in constants.go.
[CLI-1743]
Address review findings on the post-invoke hooks feature.
AddPostInvokeHook checked e.initialized outside e.mu and appended under
it, while Init stored the flag outside e.mu entirely. A concurrent Init
could land between the check and the append, registering a hook after
initialization and violating the documented contract. Both the check and
the store now happen under e.mu; Invoke's read stays lock-free and Init
still runs extension initializers unlocked, so registration from an
initializer cannot deadlock.
The hook timeout was a package-level mutable var that tests reassigned.
It now comes from Configuration via POST_INVOKE_HOOK_TIMEOUT, typed as a
time.Duration like CONFIG_CACHE_TTL, defaulting to the previous 5s so
existing callers are unaffected. Consumers with different budgets can
set their own.
A hook that exceeds the timeout is abandoned and keeps running after
Invoke returns, which races with caller-owned state - confirmed with the
race detector. That behaviour is unchanged by design, but it was
undocumented and invisible: the PostInvokeHook contract now states it,
and the timeout warning reports how many hooks were still running.
[CLI-1743]
firePostInvokeHooks read POST_INVOKE_HOOK_TIMEOUT from the engine-wide
Configuration, but Invoke resolves a per-invocation config into
options.config - a clone of the engine config by default, replaced
outright when the caller passes WithConfig. A timeout set on a config
handed to WithConfig was therefore ignored, while the workflow and
network layers honoured that same config.
Thread options.config into firePostInvokeHooks and resolve the timeout
from it, falling back to the engine config when it is nil. The nil case
is real: options.config is only defaulted inside the callback branch, so
it is unset on the workflow-not-found path, which also fires hooks.
Reported by Cursor Bugbot and a reviewer on d6aea66.
[CLI-1743]
Init runs each extension initializer in turn and returns early when one
errors, leaving initialized false and nothing unwound. Because
AddPostInvokeHook only appends, a caller who retries Init re-runs every
initializer and re-registers the hooks the earlier attempt had already
added, so each top-level Invoke ran them twice.
Snapshot the hook count before the initializer loop and truncate back to
it when an initializer fails, so each attempt contributes its hooks at
most once. The snapshot is taken rather than clearing outright because
hooks registered before Init - the pkg/app WithPostInvokeHooks path -
must survive the unwind. The truncation takes e.mu, but the initializer
loop still runs unlocked, so an initializer registering a hook cannot
deadlock against it.
Reported by Cursor Bugbot.
[CLI-1743]
In firePostInvokeHooks, hooks that exceed the timeout are 'abandoned' and continue running in the background. While types.go documents this and warns against writing to caller-owned state, there is no enforcement. If a hook captures a pointer to the output data and modifies it after the timeout, it will cause a data race with the primary caller who has already received control back from Invoke. Consider deep-copying InvokeOutput or explicitly marking the interface as read-only.
In firePostInvokeHooks, hookCtx is created with a timeout derived from configuration. If the hooks complete successfully before the timeout, cancel() is called via defer. However, the 'abandoned' goroutines for timed-out hooks continue to hold a reference to hookCtx and the hookEngine (an engineWrapper). This keeps the WrappedEngine and potentially a large configuration/context tree alive in memory longer than necessary if hooks frequently hang.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
User description
Description
This PR introduces a capability to register and run post invoke hooks.
Checklist
make test)make generate)make lint)go get github.com/snyk/go-application-framework@YOUR_LATEST_GAF_COMMITin thecliv2directory.go.modto point to your local GAF code.go mod tidyin thecliv2directory.go.modandgo.sumchanges.Note
Medium Risk
Touches the central
Invokepath (panic recovery, hook goroutines, timeout) so regressions could affect every workflow run; behavior is heavily tested and hooks are opt-in registration.Overview
Adds post-invoke hooks that run after each top-level
Engine.Invokecompletes, including when the workflow is missing or the callback returns an error.Hooks receive an
InvokeOutput(workflow id, data, error) and a scopedEngine. Registration is viaAddPostInvokeHook/PostInvokeHookRegistrar, withworkflow.AddPostInvokeHookand appWithPostInvokeHooks. Hooks must be registered beforeInit; late registration errors.Nested invokes (sub-workflows via
engineWrapper/invocation.GetEngine()) do not fire hooks; the wrapper injectswithNested(). Hooks that invoke workflows through the engine argument stay nested and avoid recursion.Hooks run concurrently with a shared 5s timeout context; slow hooks are logged and
Invokedoes not wait indefinitely. Hook and callback panics are recovered so other hooks still run and panics surface as errors to hooks before the original panic is re-raised.initializedonEngineImplis nowatomic.Bool. Mocks addInvokeOutputandPostInvokeHookRegistrar.Reviewed by Cursor Bugbot for commit 2c1f800. Bugbot is set up for automated code reviews on this repo. Configure here.
PR Type
Enhancement, Bug fix
Description
Introduce post-invoke hooks for engine invocations.
Hooks run concurrently after top-level invokes.
Skip hooks for nested invocations.
Fix hook registration race condition.
Add configurable timeout for hooks.
Diagram Walkthrough
flowchart LR A[Engine.Invoke] --> B{Workflow Found}; B -- Yes --> C[Workflow Callback]; B -- No --> D[Invoke Error]; C --> E(Post Invoke Hooks); D --> E; E -- Concurrent, Timed --> F[Hook Completion]; A -- Nested Invoke --> G[Engine.Invoke (Nested)]; G --> C; G -- Hooks Skipped --> F;File Walkthrough
4 files
Add test for WithPostInvokeHooks optionAdd mocks for InvokeOutput and PostInvokeHookRegistrarAdd comprehensive tests for post-invoke hooksAdd test for hook recursion guard5 files
Add WithPostInvokeHooks option for app engineAdd POST_INVOKE_HOOK_TIMEOUT constantImplement post-invoke hook execution and timeoutMark wrapper invokes as nested to skip hooksDefine InvokeOutput, PostInvokeHook types and Registrar1 files
Improve test isolation for proxy config detection