Add valid only allowed to link to valid check

[MaximilianSoerenPollak]

📌 Description

Add valid only allowed to link to valid check

The test is currently only a 'soft warning' and not enabled. (is_new_check =true).
In future releases this should be enabled and the RST based tests enbaled as well as the metamodel.yaml graph based tests adapted.

🚨 Impact Analysis

• This change does not violate any tool requirements and is covered by existing tool requirements
• This change does not violate any design decisions
• Otherwise I have created a ticket for new tool qualification

✅ Checklist

• Added/updated documentation for new or changed features
• Added/updated tests to cover the changes
• Followed project coding standards and guidelines

[github-actions]

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run --lockfile_mode=error //src:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.4.2) and connecting to it...
INFO: Invocation ID: dbf855bb-d522-4771-89e5-eaf391d6d9ec
Computing main repo mapping: 
Loading: 
Loading: 0 packages loaded
Loading: 0 packages loaded
WARNING: Target pattern parsing failed.
ERROR: Skipping '//src:license-check': no such target '//src:license-check': target 'license-check' not declared in package 'src' defined by /home/runner/work/docs-as-code/docs-as-code/src/BUILD
ERROR: no such target '//src:license-check': target 'license-check' not declared in package 'src' defined by /home/runner/work/docs-as-code/docs-as-code/src/BUILD
INFO: Elapsed time: 5.051s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
ERROR: Build failed. Not running target

[github-actions]

The created documentation from the pull request is available at: docu-html

[a-zw]

Why filter out externals? Doesn't that mean it is ok to link invalid external needs?

[MaximilianSoerenPollak]

If you look at the check, what we want to do is we want to check that any LOCAL needs do not link to any invalid needs (local or external).

This way we will not check that external ones link badly.

[a-zw]

I'm a little worried about performance here because we will eventually have to process many needs items. Using lists this is O(n³). Just by using a set for valid_needs_id_all, this should be O(n²) which is bad enough.

[MaximilianSoerenPollak]

Not quiet sure I can follow the difference here.
Where would having a set instead of a list make a difference in this?

Yes lists isn't ideal already you are right, that's why I used the itertools chain to ensure we have the fastest way to group the links.

[a-zw]

With a list the not in check works by iterating over the list O(n). With a set, it is a hash-map-lookup O(1).

[MaximilianSoerenPollak]

Ohh, I did not know. I will adapt that for sure. I thought sets and lists are same lookup.

[a-zw]

Can we check link.get("status") == "valid" instead of checking if the id is in some list?

[MaximilianSoerenPollak]

We could theoretically, but the issue is that the link we get here (NeedLink) is not a real Need.
We would then have to use that NeedLink.id to look up the actuall need in a dictionary.

So I thought it will be faster to gather all the 'valid needs' once, and then just check the id.

[PandaeDo]

LGTM

[Copilot]

Pull request overview

Adds a new graph check check_valid_only_links_to_valid that warns (as a non-fatal "new check" info) when a need with status: valid links to a need that is not valid. A companion RST-based test fixture is added but is intentionally not yet active.

Changes:

• New @graph_check function iterating all valid local needs and reporting links targeting non-valid needs via log.warning_for_need(..., is_new_check=True).
• Imports chain from itertools and NeedLink from sphinx_needs.need_item; reaches into need._links to enumerate links.
• New RST fixture test_invalid_graph.rst defining one valid child linking to an invalid parent.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
src/extensions/score_metamodel/checks/graph_checks.py	Implements the new check_valid_only_links_to_valid graph check and supporting imports.
src/extensions/score_metamodel/tests/rst/graph/test_invalid_graph.rst	New RST test fixture for the check; both #CHECK: and #EXPECT: are commented out, so it currently does not exercise the check.

[a-zw]

Did it read my comment? 🤣

[MaximilianSoerenPollak]

Stolen valor :D

[MaximilianSoerenPollak]

There is no way around this that is better.
We already have all of the links inside the need element, I can access it like this easily.

Going around and doing it via the app.get_all_link_rypes and then CHECKING each link type if it's possible / doable in this need and valid, just seems very workaroundy and also much slower.

[AlexanderLanin]

AI-assisted review (Claude):

Critical: 0 | Important: 2 | Suggestions: 0

The check logic is sensible, but there are two issues that need attention before merge.

1. Test file has no assertions (Important)
test_invalid_graph.rst builds successfully but verifies nothing. The #EXPECT: line is inside an RST comment (..), so test_rules_file_based.py never parses it — the test framework matches line.startswith("#EXPECT:"), which is false for RST-commented lines. If the check function were deleted tomorrow, this test would still pass. The PR description calls this intentional (soft warning / is_new_check=True), but the result is a test file with zero test value. Consider either removing it until the check is promoted to a real warning, or adding a mechanism to verify INFO-level check output.

On top of this, the commented-out expected message references comp_saf_fmea__child__16, but the defined need has ID comp_saf_fmea__child__1 — so even if the line were uncommented, it would not match.

2. Dead links and invalid-status links conflated (Important)
If a valid need links to a non-existent ID (dead link), link.id not in valid_needs_id_all is also true, producing "is valid but links to invalid need: <dead-id>" when the linked need simply doesn't exist. This produces a misleading message and potentially duplicates what sphinx-needs already reports for dead links. The check should distinguish between a missing need and one with the wrong status.

On existing comments:

• The list vs set performance point raised by @a-zw is correct — valid_needs_id_all should be a set for O(1) lookup. An even cleaner fix is to drop the pre-collection entirely and resolve each link.id directly via all_needs.get(link.id), which also handles the dead-link case above and avoids the _links private attribute access.