.map() is being used for its side effects — each .triage() call mutates triageResults but the mapped array is discarded. Use .forEach() instead:

triagePlugins.forEach((p) => p.triage(bundle, triageResults))

Right now class TriageResults introduces indirection for no benefit. Let's refactor the API to either of the two approaches:

1. Simplify the interface
   Drop TriageResults, refactor interface TriagePlugin to triage: (bundle: DecodedBundle) => TriageResult[].

This will let us replace

const results = new TriageResults()
triagePlugins.map((p) => p.triage(bundle, results))
triageResults = results.results

with

triageResults = triagePlugins.flatMap((p) => p.triage(bundle))

2. Keep TriageResults class to future-proof against more complicated functionality, but make it more ergonomic
   Refactor interface TriagePlugin to triage: (bundle: DecodedBundle) => TriageResults, instead of public readonly results - get results: () => TriageRuleResult[], drop TriageResults::push;
   Add to class TriageResults static concat: (a: TriageResults, b: TriageResults) => TriageResults, which makes the code that executes the triage:

triageResults = triagePlugins.map((p) => p.triage(bundle)).reduce(TriageResults.concat, new TriageResults()).results

or

concat: (...rs: TriageResults[]) => TriageResults

triageResults = TriageResults.concat(...triagePlugins.map((p) => p.triage(bundle))).results

Option 1 is cleaner, but option 2 is better if you already know of possible future triage features beyound a simple list of results

I had a discussion with @mihaibudiu on the TriageResults class. His suggestion was that the triage rules should not return the TriageResults object because the object can just function as an accumulator directly. The current implementation no longer includes triageResults = results.results. I replaced each instance of TriageResult[] with an instance of the TriageResults class. Take a look and let me know what you think!

The thing is mutating the object passed as an argument (as an explicit feature, not an optimization) is less obvious behavior than an explicit function output. I can open a separate PR for this, but we might as well decide on it now.

I'm not against merging the PR as-is, but it is a simple change and p.triage() mutating the passed TriageResults is both less obvious behavior and increases coupling between TriageResults and .triage function. Instead of constructing a clean TriageResults instance, .triage() needs to know how to mutate it properly.

You may think this difference is inconsequential, but when the codebase becomes overgrown with implicit behavior it becomes difficult to reason about.

@mihaibudiu please approve if you are fine with the current API

I really don't see how appending to a list is different from calling a method on an object that holds the list. They both mutate the object.

The object approach is more general if you plan to do more than just add diagnostic messages. For example, you could have errors and warnings as a separate class of objects. (But for now I suggested keeping them as diagnostic messages too).

I agree that the object as a response is more future proof.

If we look at option 2 i suggested, the idea is to separate concerns: plugin.triage() just wraps its own conclusions as a TriageResults. It does not need to concern itself with any pre-existing TriageResults state. Then, aggregating all TriageResults is a separate concern

That works as long as the results are always a TriageResult.

I assumed TriageResults is there to encapsulate the set of any triage output - list of suggestions, some overview performance graph, etc.