test: make isAttestation handler work with negative tests

[markolazic01]

Motivation

Three Gloas on_attestation spec test vectors added in ethereum/consensus-specs#5275 were skipped in #9422 with a TODO pending investigation. This PR resolves that investigation and re-enables the tests.

Description

The validation logic in validateAttestationData was already correct — all three payload-status checks were implemented and firing as expected. The root cause was that the attestation step handler in fork_choice.test.ts had no valid flag handling: it called onAttestation unconditionally and let any thrown ForkChoiceError propagate as a test failure, even when the spec vector marks the attestation as valid: false.

Fix: wrap onAttestation in the same try/catch + isValid pattern already used by the execution_payload step handler, then unskip the three tests.

Closes #9447

AI Assistance Disclosure

• External Contributors: I have read the [contributor guidelines](https://github.com/ChainSafe/lodestar/blob/unstable/CONTRIBUTING.md#ai-assistance-notice) and disclosed my usage of AI below.

Used Claude as a debugging collaborator to trace the failure from error output to root cause and to draft the descriptions. Code changes authored manually.

[gemini-code-assist]

Code Review

This pull request updates the fork choice spec tests to handle negative test cases for attestations by wrapping the onAttestation call in a try-catch block. It also re-enables several previously skipped validate_on_attestation tests. Feedback on these changes suggests moving the test setup (getIndexedAttestation) outside of the try-catch block to prevent false positives, and using safe type narrowing (instanceof Error) instead of unsafe type casting.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

There are two improvements we can make here:

1. Move test setup outside the try block: getIndexedAttestation is part of the test setup/harness. If it throws an error (e.g., due to a malformed state or missing epoch context), that error would be caught by the try/catch block. In a negative test (isValid is false), this setup failure would be incorrectly interpreted as a successful validation failure, leading to a false positive. Moving it outside ensures we only catch errors thrown by onAttestation itself.
2. Use safe type narrowing: Casting e as Error is unsafe if e is not an instance of Error (or is null/undefined). Using e instanceof Error is the standard and safer way to narrow the unknown error type in TypeScript.

              const indexedAttestation = headState.cachedState.epochCtx.getIndexedAttestation(ForkSeq[fork], attestation);
              try {
                chain.forkChoice.onAttestation(
                  indexedAttestation,
                  attDataRootHex
                );
                if (!isValid) throw Error("Expect error since this is a negative test");
              } catch (e) {
                if (isValid || (e instanceof Error && e.message === "Expect error since this is a negative test")) throw e;
              }

[markolazic01]

Applied 1., skipped 2. to follow the pattern from other tests.

[markolazic01]

Formatted, all checks should pass now.

[nflaig]

LGTM, thanks @markolazic01 for investigating and fixing this!

[nflaig]

AI Assistance Disclosure

feel free to omit this from your PRs going forward if you like

[markolazic01]

feel free to omit this from your PRs going forward if you like

Will do, thanks!

[wemeetagain]

🎉 This PR is included in v1.44.0 🎉