[One Workflow] fail Scout approval tests on stale approved step/trigger rows#260939
[One Workflow] fail Scout approval tests on stale approved step/trigger rows#260939skynetigor wants to merge 2 commits intoelastic:mainfrom
Conversation
- Added validation to ensure that every step returned by the server is listed in the fixture with a matching handler hash, catching new or changed handlers not yet approved. - Implemented checks to confirm that every fixture row still exists on the server with the same hash, addressing stale entries after steps or triggers are removed or lists are not updated. - Updated comments for clarity on the purpose of the new validations in both step and trigger approval tests.
skynetigor
added
release_note:skip
There was a problem hiding this comment.
Pull request overview
Adds bidirectional validation to Scout approval specs so fixture rows must correspond exactly to currently registered step/trigger definitions (detecting stale approved entries as well as unapproved server changes).
Changes:
- Add “Approved ⊆ registered” validation pass for triggers to fail on stale/mismatched fixture entries.
- Add “Approved ⊆ registered” validation pass for steps to fail on stale/mismatched fixture entries.
- Document the two directions (“Registered ⊆ approved” and “Approved ⊆ registered”) with targeted comments.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/platform/plugins/shared/workflows_extensions/test/scout/api/tests/trigger_definitions_approval.spec.ts | Adds reverse inclusion checks to detect stale approved trigger fixture rows. |
| src/platform/plugins/shared/workflows_extensions/test/scout/api/tests/step_definitions_approval.spec.ts | Adds reverse inclusion checks to detect stale approved step fixture rows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -59,6 +61,22 @@ apiTest.describe( | |||
| message: `Trigger "${trigger.id}" has an invalid schema hash`, | |||
| }).toBe(trigger.schemaHash); | |||
| } | |||
There was a problem hiding this comment.
Both directions currently do linear .find(...) inside loops, which makes the test O(n²) over the number of triggers. Consider building a Map/object index once for response.body.triggers (and optionally for APPROVED_TRIGGER_DEFINITIONS) and doing constant-time lookups; this improves runtime and avoids duplicating lookup logic in two loops.
| for (const approved of APPROVED_TRIGGER_DEFINITIONS) { | ||
| const registeredTrigger = response.body.triggers.find( | ||
| (trigger: { id: string; schemaHash: string }) => trigger.id === approved.id | ||
| ); |
There was a problem hiding this comment.
Both directions currently do linear .find(...) inside loops, which makes the test O(n²) over the number of triggers. Consider building a Map/object index once for response.body.triggers (and optionally for APPROVED_TRIGGER_DEFINITIONS) and doing constant-time lookups; this improves runtime and avoids duplicating lookup logic in two loops.
| expect(registeredTrigger, { | ||
| message: `Approved list contains stale entry "${approved.id}" that is not registered`, | ||
| }).toBeDefined(); | ||
|
|
||
| expect(registeredTrigger?.schemaHash, { | ||
| message: `Approved entry "${approved.id}" has an invalid schema hash (does not match registered trigger)`, | ||
| }).toBe(approved.schemaHash); |
There was a problem hiding this comment.
After asserting registeredTrigger is defined, the follow-up assertion still uses optional chaining (registeredTrigger?.schemaHash). This can make failures harder to interpret and weakens type safety. Prefer asserting defined and then accessing the value directly (e.g., via non-null assertion or by restructuring so the second expect only runs when defined).
| @@ -57,6 +59,22 @@ apiTest.describe( | |||
| message: `Step "${step.id}" has an invalid handler hash`, | |||
| }).toBe(approvedStep?.handlerHash); | |||
| } | |||
There was a problem hiding this comment.
As with triggers, the two loops do repeated .find(...) lookups, leading to O(n²) behavior. Consider indexing response.body.steps (and/or APPROVED_STEP_DEFINITIONS) into a Map once to make both the forward and reverse checks O(n) and reduce duplicated lookup logic.
| for (const approved of APPROVED_STEP_DEFINITIONS) { | ||
| const registeredStep = response.body.steps.find( | ||
| (step: { id: string; handlerHash: string }) => step.id === approved.id | ||
| ); |
There was a problem hiding this comment.
As with triggers, the two loops do repeated .find(...) lookups, leading to O(n²) behavior. Consider indexing response.body.steps (and/or APPROVED_STEP_DEFINITIONS) into a Map once to make both the forward and reverse checks O(n) and reduce duplicated lookup logic.
| message: `Approved list contains stale entry "${approved.id}" that is not registered`, | ||
| }).toBeDefined(); | ||
|
|
||
| expect(registeredStep?.handlerHash, { |
There was a problem hiding this comment.
Similar to the trigger spec, optional chaining in the second assertion (registeredStep?.handlerHash) is unnecessary after asserting registeredStep is defined and can obscure intent/type safety. Prefer direct access after the defined-assertion (e.g., using non-null assertion or control flow that guarantees presence).
| expect(registeredStep?.handlerHash, { | |
| expect(registeredStep!.handlerHash, { |
botelastic
bot
added
the
Team:One Workflow
💔 Build Failed
Failed CI Steps
Test Failures
Metrics [docs]
History |
Summary
Scout approval specs already required every registered step/trigger (from the internal API) to appear in
approved_{step,trigger}_definitions.tswith a matching hash. They did not require the reverse: rows left in those fixtures after a definition was removed from the server still passed.This change adds a second validation pass so every fixture row must still exist on the server with the same
handlerHash/schemaHash, and documents both directions with short comments (Registered ⊆ approvedvsApproved ⊆ registered).Changes
step_definitions_approval.spec.ts— After the existing loop overresponse.body.steps, iterateAPPROVED_STEP_DEFINITIONSand assert each id is returned by the API and the hash matches; clear failure messages for stale or mismatched entries. Comment above the original loop explains the forward check.trigger_definitions_approval.spec.ts— Same pattern forAPPROVED_TRIGGER_DEFINITIONSandschemaHash.