Skip to content

feat(event-handlers): introduce event handler system for GitProxy lifecycle events#1519

Open
Andreybest wants to merge 7 commits into
finos:mainfrom
Andreybest:1449-event-hooks
Open

feat(event-handlers): introduce event handler system for GitProxy lifecycle events#1519
Andreybest wants to merge 7 commits into
finos:mainfrom
Andreybest:1449-event-hooks

Conversation

@Andreybest

@Andreybest Andreybest commented May 5, 2026

Copy link
Copy Markdown
Contributor

Implements #1449.

Introduces a lightweight, async, observer-only event system so external integrations (notifications, audit feeds) can react to push/pull lifecycle events without coupling to chain internals.

Event hooks can be added the same way as the plugins in config.proxy.json

Supports 2 operations: pull and push
With 5 phases:

  • started
  • completed
  • error
  • permissionDenied
  • pendingReview

This is not well documented, and not sure where to add info on that, would appreciate suggestions on that matter :)

@Andreybest Andreybest requested a review from a team as a code owner May 5, 2026 23:11
@netlify

netlify Bot commented May 5, 2026

Copy link
Copy Markdown

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 3948487
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/6a314058ea1b13000919c7ca

@Andreybest Andreybest requested a review from grovesy May 5, 2026 23:11
@codecov

codecov Bot commented May 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.56913% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.81%. Comparing base (656d3f5) to head (3948487).

Files with missing lines Patch % Lines
src/eventHandlers/loader.ts 91.17% 6 Missing ⚠️
src/eventHandlers/builtin/consoleLogger.ts 88.63% 5 Missing ⚠️
src/proxy/chain.ts 75.00% 5 Missing ⚠️
src/eventHandlers/dispatcher.ts 97.67% 1 Missing and 1 partial ⚠️
src/eventHandlers/index.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1519      +/-   ##
==========================================
+ Coverage   85.51%   85.81%   +0.30%     
==========================================
  Files          83       89       +6     
  Lines        7877     8187     +310     
  Branches     1312     1378      +66     
==========================================
+ Hits         6736     7026     +290     
- Misses       1114     1133      +19     
- Partials       27       28       +1     

☔ 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.

@andypols

andypols commented May 12, 2026

Copy link
Copy Markdown
Contributor

@Andreybest Could I vote for an additional phase?

We have several custom actions that run when a push is first received and the proxy blocks it for review. This state is not really permissionDenied or error; it is closer to pendingReview.

We also have custom actions that would work really well as plugin events. For example, we:

  • run an AI-based security audit of the changes
  • create an AI summary of the changes to help the reviewer
  • notify reviewers when there is a push to review

It would be great if plugins could also add data that can be rendered in the UI.

This would be a hugely helpful change, as it would allow us to separate our custom code from the base git-proxy. Keeping the two in sync is painful at the moment.

@Andreybest

Copy link
Copy Markdown
Contributor Author

Hey @andypols ! Thanks for the suggestion, this seems like a good use cases for events. Will try to add mechanisms that would be able to support your use cases :)
Will ping you on that later.

@re-vlad re-vlad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a wish: while the code is well-documented, there's no external docs (README, docs, etc.) explaining how to use the event system. This should be added.

Comment thread proxy.config.json
"contactEmail": "",
"csrfProtection": true,
"plugins": [],
"eventHandlers": [],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The config schema shows the eventHandlers array, but there are no examples of how to configure event handlers in proxy.config.json. Consider to document example

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added documentation for that in architecture.md. Thanks!

if (!action.user && !action.userEmail) {
return undefined;
}
return { username: action.user, email: action.userEmail };

@re-vlad re-vlad May 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

EventDetails.user and repository must be immutable copies (using String()) of Action fields — not references. Otherwise, async handlers may see stale or modified data if Action is mutated later in the chain. This rather critical I believe, but easy to fix:

return { 
  username = action.user ? String(action.user) : undefined;
  email = action.userEmail ? String(action.userEmail) : undefined;
}```. 

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Both values are strings, and strings are copied by value, not by reference, thus why this is unnecessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think Vlad's point here is that there is no object-level protection, in other words the fields can be overwritten unless we enforced the contract via something like Object.freeze(details.user) after defining details below.

For example, the following event handler would affect any subsequent handlers that use the EventDetails:

export default new EventHandlerPlugin(() => {
  const redact = (details: EventDetails) => {
    if (details.user) {
      details.user.email = '<redacted>';
    }
    sendTo3rdPartyApp(details);
  };
  registry.onPush().onCompleted(redact);
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right, haven't thought about that, but I don't think that we should freeze object. This is proper, im not against it, but probably plugin code or dependencies will change object (which I do not agree they should do that), so I made a cloning of EventDetails object for each handler invocation.

} from './types';

const buildRepositoryContext = (action: Action): RepositoryContext => ({
url: action.url,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The same problem like line 38, don't return references, return values instead:

name: action.repoName? String(action.repoName) : undefined,
url: String(action.url),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Responded on comment above

@Andreybest Andreybest requested a review from re-vlad May 21, 2026 02:17
@Andreybest

Andreybest commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

@Andreybest Could I vote for an additional phase?

We have several custom actions that run when a push is first received and the proxy blocks it for review. This state is not really permissionDenied or error; it is closer to pendingReview.

We also have custom actions that would work really well as plugin events. For example, we:

  • run an AI-based security audit of the changes
  • create an AI summary of the changes to help the reviewer
  • notify reviewers when there is a push to review

It would be great if plugins could also add data that can be rendered in the UI.

This would be a hugely helpful change, as it would allow us to separate our custom code from the base git-proxy. Keeping the two in sync is painful at the moment.

Hey @andypols! Added a new phase - pendingReview. Which indicates that the Action is blocked and not autoApproved and not autoRejected

On "add data that can be rendered in the UI", not sure if this should be the part of this PR, since the current architecture of event hooks is to inform a plugin that something happened in a manner of "fire-and-forget". And as I understand this will also require changes to database schema itself.

Please let me know if I understood what you have meant. :)

@andypols

Copy link
Copy Markdown
Contributor

Hey @andypols! Added a new phase - pendingReview. Which indicates that the Action is blocked and not autoApproved and not autoRejected

On "add data that can be rendered in the UI", not sure if this should be the part of this PR, since the current architecture of event hooks is to inform a plugin that something happened in a manner of "fire-and-forget". And as I understand this will also require changes to database schema itself.

Please let me know if I understood what you have meant. :)

@Andreybest Thats sounds perfect. It makes sense that the UI is in a different PR, although not sure about the need for a scheme change (we have this and did not need one)

@jescalada jescalada left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Andreybest Thanks for the contribution! It's looking great so far.

It'd be great to have a simple doc on how to configure a preexisting handler (such as the consoleLogger), and how to develop a new one. A short step-by-step guide will make it easier for everyone to adopt.

It'd be fantastic to have an extra builtin handler for sending emails as mentioned in #1121, although we could add it in a separate PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a great example file to use for the Event Handler documentation 🙂

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Added information on this example as a separate page in website docs.

Comment thread src/eventHandlers/types.ts Outdated
* operations. For blocking/policy logic, write a chain plugin instead.
*
* Handlers run asynchronously, fire-and-forget. Errors thrown by a handler
* are caught and logged but never propagate to the git response.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These details would be best explained in a separate document - things like when to choose an event handler over a plugin as they're superficially similar 👍🏼

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Added information on this example as a separate page in website docs.

errorMessage?: string | null;
blocked: boolean = false;
blockedMessage?: string | null;
permissionDenied: boolean = false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand permissionDenied is set exclusively when checkUserPushPermission fails. I wonder if there's a more elegant solution - perhaps storing the name of the step where the Action failed. We could also infer this from the lastStep so we wouldn't need the extra field.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see the value of handling the specific case of permissionDenied. I'm just wondering if we could prevent future bloat from happening as people try to add event handling to other, arbitrary push actions.

OTOH, I'd say it's okay to keep it if it makes the event handler implementation more obvious.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Lets keep it like this for now :)

else if (phase === 'completed') builder.onCompleted(log);
else if (phase === 'pendingReview') builder.onPendingReview(log);
else if (phase === 'error') builder.onError(log);
else if (phase === 'permissionDenied') builder.onPermissionDenied(log);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might want to have an else to handle things if someone added an additional ActionPhase but forgot to update the switch here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thanks for pointing out! Added!

if (!action.user && !action.userEmail) {
return undefined;
}
return { username: action.user, email: action.userEmail };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think Vlad's point here is that there is no object-level protection, in other words the fields can be overwritten unless we enforced the contract via something like Object.freeze(details.user) after defining details below.

For example, the following event handler would affect any subsequent handlers that use the EventDetails:

export default new EventHandlerPlugin(() => {
  const redact = (details: EventDetails) => {
    if (details.user) {
      details.user.email = '<redacted>';
    }
    sendTo3rdPartyApp(details);
  };
  registry.onPush().onCompleted(redact);
});

@Andreybest

Copy link
Copy Markdown
Contributor Author

Thanks for the review @jescalada! Added changes to your comments. On matter of built in handler for email notifications - I suggest put it as a separate PR, since the specifications for events are not 100% final and could be changed later in this PR :)

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.

4 participants