Feat: Forbid certain uses of need extends directive

[MaximilianSoerenPollak]

📌 Description

Adds capability to check needextend usage and throw warnings if used in a way we do not allow

Fixes: #345

🚨 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.6.0) and connecting to it...
INFO: Invocation ID: 5b338e73-d58c-42e9-bc33-20504b607014
Computing main repo mapping: 
Loading: 
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
    currently loading: src
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.626s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
ERROR: Build failed. Not running target

[a-zw]

The word is "immutable".

[github-actions]

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

[AlexanderLanin]

AI-assisted review (Claude):

Critical: 0 | Important: 2 | Suggestions: 3

---

Overall

Solid addition — the monkey-patch target is correct (sphinx_needs.directives.need imports and calls extend_needs_data locally, so patching that module's namespace intercepts the call). The approach of wrapping the upstream function and delegating is clean.

Two concerns worth addressing before merge:

1. The check cannot be disabled via score_metamodel_checks — it fires unconditionally at import time rather than integrating with the existing check registry.
2. Three of the five validation branches have no test coverage — link REPLACE/DELETE, field DELETE, and APPEND on string options are all untested.

See inline comments for details.

[AlexanderLanin]

AI-assisted review (Claude):

Important: Three validation branches have no test coverage

The checker has five distinct warning paths. Only two are exercised here:

Branch	Covered?
list_modifications: REPLACE/DELETE on links	❌
modifications: DELETE	❌
modifications: APPEND on string option	❌
modifications: REPLACE when option already set	✅ (line 23)
modifications: REPLACE when option not set	✅ (line 32)

The three uncovered branches represent ~60% of the warning logic. Adding RST fixtures for at least list_modifications replace-on-links and DELETE would significantly increase confidence that the logic works as intended.

[AlexanderLanin]

AI-assisted review (Claude):

Suggestion: external-need branch warns but continues processing

After logging the warning for an external need, the loop falls through and still executes the modification checks below (lines 89–130) for that same need. The original extend_needs_data will then apply the actual change. This means the user gets a warning and the modification still happens.

If modifying external needs is genuinely disallowed, this should continue after the warning (or the original function should skip it too). If it's just advisory for now, the message "It is not allowed" is misleading — consider "Warning: modifying external needs is discouraged" until it's enforced.

[MaximilianSoerenPollak]

Ye that's not an issue, you get the warning and the build fails.
Though we could do early exits to speed it up, that mightn ot be a bad idea.

[AlexanderLanin]

AI-assisted review (Claude):

Suggestion: location is re-assigned inside the per-need loop, shadowing the outer variable

location is first set on line 44 for the current needextend directive, then reassigned on lines 91–94 inside the for found_need in found_needs loop. The second assignment is identical to the first — it just copies the same values. This creates a subtle trap: if a future change needs the needextend's location before the inner loop, the pre-loop assignment on line 44 looks authoritative but gets clobbered. Remove the redundant inner reassignment.

[MaximilianSoerenPollak]

That is taken stragith from the function. I'm not changing that, it is how the original function works.

[AlexanderLanin]

AI-assisted review (Claude):

Critical: # EXPECT: (space) is silently ignored by the test parser

Line 53 uses # EXPECT: with a space after #. The parser in test_rules_file_based.py checks line.startswith("#EXPECT:") — the space breaks the match, so this annotation is parsed as an ordinary RST comment and discarded.

Result: the list_modifications REPLACE/DELETE branch (check_needs_extends.py lines 96–107) has no effective test. A bug in that branch would not be caught.

Fix: remove the space.

Suggested change

	# EXPECT: Replace or Delete action is not allowed via needextends.
	#EXPECT: Replace or Delete action is not allowed via needextends.

[AlexanderLanin]

AI-assisted review (Claude):

Critical: 1 | Important: 0 | Suggestions: 0

---

New finding

One new critical issue found in the latest commits: line 53 of test_need_extends.rst uses # EXPECT: (with a space) instead of #EXPECT:. The parser uses line.startswith("#EXPECT:") so the space causes the annotation to be silently dropped. The REPLACE/DELETE links branch in check_needs_extends.py (lines 96–107) therefore has no effective test. See inline comment for a one-line suggestion fix.

---

Responses to previous review pushback

Registry integration (my previous Important finding): The author's response — "this is a wrapper, it has to take those arguments" — misses the point. The concern isn't the function signature; it's that the check fires unconditionally regardless of score_metamodel_checks. A minimal fix could be: read app.config.score_metamodel_checks at call time inside score_extend_needs_data_func and return early if this check is excluded. No signature change needed. That said, if the project is OK with this check being permanently always-on and non-filterable, that's a policy decision — but it should be explicit, not accidental.

External-need fall-through (my previous Suggestion): The author says "you get the warning and the build fails." If the build already fails, the fall-through is harmless — but the word "Error" in the warning message combined with processing continuing is still confusing to readers of the log. A continue or a message reword is still worth the line.

Redundant location reassignment (my previous Suggestion): The author says it's taken straight from the original function. That's fine as a reason to not change it — accepted.

---

Unchanged issues

Typos in docs/how-to/write_docs.rst (lines 37–40: documentaiton, uscases) were not fixed. Still worth a one-liner before merge.

[AlexanderLanin]

WARNING: Error. It is not allowed to modify external needs via needextend should print which need was attempted to modify, that will help improving the filter. Actually this should include a hint to use c.this_doc ?!

[AlexanderLanin]

AI-assisted review (Claude):

Critical: 0 | Important: 0 | Suggestions: 1

---

The two new commits resolve the critical issue and the test coverage gap:

• The # EXPECT: space bug is fixed — all five branches now use correct #EXPECT: annotations.
• All five warning paths have explicit test coverage with correct expected strings (verified that EXPECT substrings match the produced error messages).
• Error messages now consistently include the need ID, which is an improvement over the previous generic messages.

One thing still open from the prior round: doc typos in docs/how-to/write_docs.rst — line 37 (documentaiton) and line 40 (Allowed uscases:). These have been raised twice now and are a one-line fix.

Otherwise this looks ready.

[AlexanderLanin]

AI fix-commit (Claude): 7ae33374 — fix typos: documentaiton → documentation, uscases → usecases