Fix variable shadowing crash in get_by_tags and code quality improvements by BEAST04289 · Pull Request #837 · OWASP/OpenCRE

BEAST04289 · 2026-03-29T13:43:36Z

Summary

Fixes a crash bug and several code quality issues found during codebase review for GSoC 2026 Module B proposal.

Closes #836

Changes

Bug Fix: Variable shadowing crash in get_by_tags() (application/database/db.py)

The loop variable node was overwritten by the return value of self.get_nodes(). When get_nodes() returned [] (empty list), the error handler tried to access node.name on the empty list, causing AttributeError. Renamed to db_node and resolved to preserve the original DB node reference for error logging.

Code Quality Fixes

Unreachable code in add_node() (db.py:1585): Removed dead return None after raise ValueError
- Duplicate import (web_main.py:15,20): Removed redundant from application.utils import oscal_utils, redis
- Variable typo (gap_analysis.py:44-46): Renamed pentalty to penalty
- Variable typo (cre_main.py:280): Renamed pending_stadards to pending_standards (4 occurrences)

Testing

All changes are localized and do not alter behavior
- The variable shadowing fix preserves the original node reference for correct error logging

…rror The loop variable 'node' was overwritten by the return value of self.get_nodes(). When get_nodes() returned [] (empty list), the error handler tried to access node.name on the empty list, causing AttributeError. Renamed to 'db_node' and 'resolved' to preserve the original DB node reference for error logging. Fixes OWASP#836

Copilot

Pull request overview

Fixes a crash in Node_collection.get_by_tags() caused by variable shadowing, and applies a few small code-quality cleanups discovered during review (typos, dead code, redundant import).

Changes:

Prevent variable shadowing in get_by_tags() so error logging can’t crash when get_nodes() returns an empty list.
Remove unreachable code in add_node() and clean up redundant imports / misspelled local variable names.
Rename typos in variables used by gap analysis and standard registration flow.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
application/database/db.py	Fixes `get_by_tags()` variable shadowing crash; removes unreachable `return` after `raise`.
application/web/web_main.py	Removes redundant `oscal_utils, redis` import line (keeps the remaining import).
application/utils/gap_analysis.py	Fixes misspelled local variable `pentalty` → `penalty`.
application/cmd/cre_main.py	Fixes `pending_stadards` typo and updates references to the corrected variable name.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-29T13:46:34Z

application/cmd/cre_main.py

                    logger.error(
                        f"Could not find gap analysis job for for {importing_name} and {standard_name} putting {standard_name} back in the queue"
                    )


Typo in log message: duplicated word for ("job for for …").

Copilot · 2026-03-29T13:46:35Z

application/cmd/cre_main.py

                    logger.error(
                        f"Could not find gap analysis job for for {importing_name} and {standard_name} putting {standard_name} back in the queue"
                    )


Typo in log message: duplicated word for ("job for for …").

Copilot · 2026-03-29T13:46:35Z

application/cmd/cre_main.py

@@ -295,7 +295,7 @@ def register_standard(
                    logger.error(
                        f"Could not find gap analysis job for for {importing_name} and {standard_name} putting {standard_name} back in the queue"
                    )
-                    pending_stadards.append(standard_name)
+                    pending_standards.append(standard_name)



This code appends back into pending_standards while iterating over it. Mutating the list during iteration can lead to unexpected behavior (including repeatedly re-processing the same standard if the exception keeps happening). Consider using a separate retry list/queue (e.g., to_retry.append(...) then extend after the loop), or iterating over a snapshot copy.

Copilot · 2026-03-29T13:46:35Z

application/cmd/cre_main.py

@@ -310,7 +310,7 @@ def register_standard(
                    logger.error(
                        f"Could not find gap analysis job for for {importing_name} and {standard_name} putting {standard_name} back in the queue"
                    )
-                    pending_stadards.append(standard_name)
+                    pending_standards.append(standard_name)
        redis.wait_for_jobs(jobs)


This code appends back into pending_standards while iterating over it. Mutating the list during iteration can lead to unexpected behavior (including repeatedly re-processing the same standard if the exception keeps happening). Consider using a separate retry list/queue (e.g., to_retry.append(...) then extend after the loop), or iterating over a snapshot copy.

Copilot · 2026-03-29T13:46:35Z

application/database/db.py

            else:
                logger.fatal(
-                    "db.get_node returned None for"
+                    "db.get_node returned None for "


The fatal log message in this branch is misleading: get_nodes() never returns None (it returns an empty list when nothing matches), and the message references db.get_node rather than get_nodes. This makes the log inaccurate and can confuse debugging; consider updating the wording to reflect an empty result from get_nodes() (and the correct function name).

Suggested change

"db.get_node returned None for "

"get_nodes() returned no documents for "

Copilot · 2026-03-29T13:46:35Z

application/database/db.py

        nodes = Node.query.filter(*nodes_where_clause).all() or []
-        for node in nodes:
-            node = self.get_nodes(
-                name=node.name,
-                section=node.section,
-                subsection=node.subsection,
-                version=node.version,
-                link=node.link,
-                ntype=node.ntype,
-                sectionID=node.section_id,
+        for db_node in nodes:
+            resolved = self.get_nodes(
+                name=db_node.name,
+                section=db_node.section,
+                subsection=db_node.subsection,
+                version=db_node.version,
+                link=db_node.link,
+                ntype=db_node.ntype,
+                sectionID=db_node.section_id,
            )
-            if node:
-                documents.extend(node)
+            if resolved:
+                documents.extend(resolved)
            else:
                logger.fatal(
-                    "db.get_node returned None for"
+                    "db.get_node returned None for "
                    "Node %s:%s:%s that exists, BUG!"
-                    % (node.name, node.section, node.section_id)
+                    % (db_node.name, db_node.section, db_node.section_id)
                )


There is test coverage for get_by_tags() (see application/tests/db_test.py::test_get_by_tags), but it doesn’t exercise the failure mode fixed here (where get_nodes() returns an empty list for a DB row that matched the initial tag query). Please add a regression test that simulates get_nodes() returning [] for a matching Node row to ensure get_by_tags() doesn’t crash when logging the error path.

…_standards

BEAST04289 added 4 commits March 29, 2026 19:03

fix(db): remove unreachable code after raise in add_node

524b5a6

fix(web): remove duplicate import in web_main.py

c7108b9

fix(gap-analysis): rename misspelled variable pentalty to penalty

4f4390c

Copilot AI review requested due to automatic review settings March 29, 2026 13:43

Copilot started reviewing on behalf of BEAST04289 March 29, 2026 13:44 View session

Copilot AI reviewed Mar 29, 2026

View reviewed changes

fix(cre-main): rename misspelled variable pending_stadards to pending…

7ae6362

…_standards

BEAST04289 force-pushed the fix/code-quality-improvements branch from 276bb39 to 7ae6362 Compare March 29, 2026 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix variable shadowing crash in get_by_tags and code quality improvements#837

Fix variable shadowing crash in get_by_tags and code quality improvements#837
BEAST04289 wants to merge 5 commits intoOWASP:mainfrom
BEAST04289:fix/code-quality-improvements

BEAST04289 commented Mar 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	"db.get_node returned None for "
	"get_nodes() returned no documents for "

Conversation

BEAST04289 commented Mar 29, 2026

Summary

Changes

Bug Fix: Variable shadowing crash in get_by_tags() (application/database/db.py)

Code Quality Fixes

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants