Add standalone OWASP AI and Top 10 resource importers#960
Open
Bornunique911 wants to merge 1 commit into
Open
Conversation
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds four new OWASP parser modules (Top 10 2025, API Top 10 2023, LLM Top 10 2025, AISVS) implementing the parser interface with bundled JSON datasets, wires CLI flags and resource registration in cre.py and cre_main.py, and adds corresponding unit tests validating parsed section counts, IDs, and CRE links. ChangesNew OWASP parsers and import wiring
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (7)
application/utils/external_project_parsers/parsers/owasp_llm_top10_2025.py (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd return type annotation for mypy compliance.
Same missing
-> ParseResultannotation as flagged inowasp_top10_2025.py. As per coding guidelines, "Runmake mypyfor Python type checking".🔧 Proposed fix
- def parse(self, cache: db.Node_collection, ph: prompt_client.PromptHandler): + def parse(self, cache: db.Node_collection, ph: prompt_client.PromptHandler) -> ParseResult:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/utils/external_project_parsers/parsers/owasp_llm_top10_2025.py` at line 19, The parse method in OWASP LLM Top 10 2025 is missing its return type annotation, which breaks mypy compliance. Update the parse signature in the parser class to explicitly declare the ParseResult return type, matching the pattern used in owasp_top10_2025.py and the surrounding parser interfaces.Source: Coding guidelines
application/utils/external_project_parsers/parsers/owasp_top10_2025.py (3)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd return type annotation for mypy compliance.
parseis missing a-> ParseResultreturn annotation, unlike the baseParserInterface.parsesignature. As per coding guidelines, "Runmake mypyfor Python type checking".🔧 Proposed fix
- def parse(self, cache: db.Node_collection, ph: prompt_client.PromptHandler): + def parse(self, cache: db.Node_collection, ph: prompt_client.PromptHandler) -> ParseResult:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/utils/external_project_parsers/parsers/owasp_top10_2025.py` at line 19, The parse method in OWASPTop10 2025 parser is missing the return type annotation required to match ParserInterface.parse and satisfy mypy. Update the parse signature on the OWASPTop10 parser class to explicitly return ParseResult, keeping the existing parameters cache and ph unchanged, so it aligns with the base interface and type checking expectations.Source: Coding guidelines
31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent skip hides unresolved
cre_idmappings.If a
cre_idin the bundled JSON doesn't resolve to an existing CRE (e.g., typo or CRE not yet imported), the link is silently dropped with no signal. A debug/warning log here would make data issues in the bundled JSON easier to catch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/utils/external_project_parsers/parsers/owasp_top10_2025.py` around lines 31 - 34, The CRE lookup loop in the OWASP Top 10 parser silently drops unresolved mappings, so update the logic in the parser that iterates over entry.get("cre_ids", []) to emit a debug or warning when cache.get_CREs(external_id=cre_id) returns nothing. Use the existing parser flow and relevant symbols like the OWASP Top 10 parsing function and cache.get_CREs to add a log message that includes the missing cre_id and enough context to spot bad bundled JSON data.
13-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate parsing logic across OWASP parsers.
This class's
parse()(Lines 19-47) is line-for-line identical in structure toOwaspLlmTop10_2025.parse()inowasp_llm_top10_2025.py, differing only inname/data_file. Per the PR stack, two more parsers (owasp_api_top10_2023,owasp_aisvs) likely follow the same shape. Consider extracting the shared "load JSON → buildStandard→ linkcre_ids" logic into a small helper (e.g., inbase_parser_defs.pyor a new mixin) parameterized byname/data_file, to avoid four parsers drifting independently as bugs are fixed.♻️ Example shared-helper approach
class JsonStandardParser(ParserInterface): data_file: Path def parse(self, cache: db.Node_collection, ph: prompt_client.PromptHandler) -> ParseResult: with self.data_file.open("r", encoding="utf-8") as handle: raw_entries = json.load(handle) entries = [] for entry in raw_entries: standard = defs.Standard( name=self.name, sectionID=entry["section_id"], section=entry["section"], hyperlink=entry["hyperlink"], ) for cre_id in entry.get("cre_ids", []): cres = cache.get_CREs(external_id=cre_id) if not cres: continue standard.add_link( defs.Link(ltype=defs.LinkTypes.LinkedTo, document=cres[0].shallow_copy()) ) entries.append(standard) return ParseResult( results={self.name: entries}, calculate_gap_analysis=False, calculate_embeddings=False, ) class OwaspTop10_2025(JsonStandardParser): name = "OWASP Top 10 2025" data_file = Path(__file__).resolve().parent.parent / "data" / "owasp_top10_2025.json"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/utils/external_project_parsers/parsers/owasp_top10_2025.py` around lines 13 - 47, The parse() implementation in OwaspTop10_2025 duplicates the same JSON-to-Standard mapping logic used by the other OWASP parser classes, so extract the shared “load JSON, build defs.Standard, attach cre_ids links” flow into a common helper or mixin (for example a base parser in base_parser_defs.py). Keep only the per-parser name and data_file differences in OwaspTop10_2025.parse() and have OwaspLlmTop10_2025, owasp_api_top10_2023, and owasp_aisvs reuse the shared implementation to prevent future drift.application/utils/external_project_parsers/parsers/owasp_api_top10_2023.py (2)
13-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate parser logic across OWASP modules.
This class is structurally identical to
OwaspAisvs(and presumably the other two new parsers): load JSON → buildStandard→ resolvecre_idsviacache.get_CREs→ link → return aParseResultwith gap analysis/embeddings disabled. Consider extracting a shared base class or helper function (e.g., takingname,data_fileas class attributes and a commonparse()implementation) to avoid maintaining four near-identical copies.♻️ Sketch of a shared base implementation
class OwaspJsonStandardParser(ParserInterface): data_file: Path def parse(self, cache: db.Node_collection, ph: prompt_client.PromptHandler) -> ParseResult: with self.data_file.open("r", encoding="utf-8") as handle: raw_entries = json.load(handle) entries = [] for entry in raw_entries: standard = defs.Standard( name=self.name, sectionID=entry["section_id"], section=entry["section"], hyperlink=entry["hyperlink"], ) for cre_id in entry.get("cre_ids", []): cres = cache.get_CREs(external_id=cre_id) if not cres: continue standard.add_link( defs.Link(ltype=defs.LinkTypes.LinkedTo, document=cres[0].shallow_copy()) ) entries.append(standard) return ParseResult( results={self.name: entries}, calculate_gap_analysis=False, calculate_embeddings=False, ) class OwaspApiTop10_2023(OwaspJsonStandardParser): name = "OWASP API Security Top 10 2023" data_file = Path(__file__).resolve().parent.parent / "data" / "owasp_api_top10_2023.json"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/utils/external_project_parsers/parsers/owasp_api_top10_2023.py` around lines 13 - 47, The OWASP parser classes are duplicating the same JSON-to-Standard parsing flow, so extract the shared implementation from OwaspApiTop10_2023 and its sibling parsers into a common base class or helper. Move the repeated logic for loading data_file, building defs.Standard objects, resolving cre_ids through cache.get_CREs, adding defs.Link entries, and returning ParseResult into a reusable parse() on a shared parser type, with each concrete class only supplying name and data_file.
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd return type annotation.
parsedoesn't declare-> ParseResult, unlike the interface it implements. As per coding guidelines,make mypyis run for type checking, so annotating consistently helps mypy catch mismatches.Diff
- def parse(self, cache: db.Node_collection, ph: prompt_client.PromptHandler): + def parse(self, cache: db.Node_collection, ph: prompt_client.PromptHandler) -> ParseResult:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/utils/external_project_parsers/parsers/owasp_api_top10_2023.py` at line 19, The parse method in OwaspApiTop10Parser is missing the explicit ParseResult return annotation required by the interface it implements. Update the parse signature in the parser class to include the same return type as the base contract, keeping the existing cache and ph parameters unchanged, so the method matches the expected typing used by mypy.Source: Coding guidelines
application/utils/external_project_parsers/parsers/owasp_aisvs.py (1)
17-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd return type annotation to
parse.
parsereturnsParseResultbut lacks a return type annotation, weakening mypy's ability to verify theParserInterfacecontract.🔧 Proposed fix
- def parse(self, cache: db.Node_collection, ph: prompt_client.PromptHandler): + def parse( + self, cache: db.Node_collection, ph: prompt_client.PromptHandler + ) -> ParseResult:As per coding guidelines, "Run
make mypyfor Python type checking" for**/*.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/utils/external_project_parsers/parsers/owasp_aisvs.py` around lines 17 - 45, The parse method in owasp_aisvs.py is missing a return type annotation even though it returns ParseResult. Update the parse signature on the Parser implementation to explicitly annotate the return type as ParseResult so mypy can verify the ParserInterface contract, keeping the existing cache and ph parameters unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@application/utils/external_project_parsers/parsers/owasp_aisvs.py`:
- Around line 17-45: The parse method in owasp_aisvs.py is missing a return type
annotation even though it returns ParseResult. Update the parse signature on the
Parser implementation to explicitly annotate the return type as ParseResult so
mypy can verify the ParserInterface contract, keeping the existing cache and ph
parameters unchanged.
In `@application/utils/external_project_parsers/parsers/owasp_api_top10_2023.py`:
- Around line 13-47: The OWASP parser classes are duplicating the same
JSON-to-Standard parsing flow, so extract the shared implementation from
OwaspApiTop10_2023 and its sibling parsers into a common base class or helper.
Move the repeated logic for loading data_file, building defs.Standard objects,
resolving cre_ids through cache.get_CREs, adding defs.Link entries, and
returning ParseResult into a reusable parse() on a shared parser type, with each
concrete class only supplying name and data_file.
- Line 19: The parse method in OwaspApiTop10Parser is missing the explicit
ParseResult return annotation required by the interface it implements. Update
the parse signature in the parser class to include the same return type as the
base contract, keeping the existing cache and ph parameters unchanged, so the
method matches the expected typing used by mypy.
In `@application/utils/external_project_parsers/parsers/owasp_llm_top10_2025.py`:
- Line 19: The parse method in OWASP LLM Top 10 2025 is missing its return type
annotation, which breaks mypy compliance. Update the parse signature in the
parser class to explicitly declare the ParseResult return type, matching the
pattern used in owasp_top10_2025.py and the surrounding parser interfaces.
In `@application/utils/external_project_parsers/parsers/owasp_top10_2025.py`:
- Line 19: The parse method in OWASPTop10 2025 parser is missing the return type
annotation required to match ParserInterface.parse and satisfy mypy. Update the
parse signature on the OWASPTop10 parser class to explicitly return ParseResult,
keeping the existing parameters cache and ph unchanged, so it aligns with the
base interface and type checking expectations.
- Around line 31-34: The CRE lookup loop in the OWASP Top 10 parser silently
drops unresolved mappings, so update the logic in the parser that iterates over
entry.get("cre_ids", []) to emit a debug or warning when
cache.get_CREs(external_id=cre_id) returns nothing. Use the existing parser flow
and relevant symbols like the OWASP Top 10 parsing function and cache.get_CREs
to add a log message that includes the missing cre_id and enough context to spot
bad bundled JSON data.
- Around line 13-47: The parse() implementation in OwaspTop10_2025 duplicates
the same JSON-to-Standard mapping logic used by the other OWASP parser classes,
so extract the shared “load JSON, build defs.Standard, attach cre_ids links”
flow into a common helper or mixin (for example a base parser in
base_parser_defs.py). Keep only the per-parser name and data_file differences in
OwaspTop10_2025.parse() and have OwaspLlmTop10_2025, owasp_api_top10_2023, and
owasp_aisvs reuse the shared implementation to prevent future drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7bac8fd0-698c-4ed9-a6ce-11b1f6fb025b
📒 Files selected for processing (14)
application/cmd/cre_main.pyapplication/tests/owasp_aisvs_parser_test.pyapplication/tests/owasp_api_top10_2023_parser_test.pyapplication/tests/owasp_llm_top10_2025_parser_test.pyapplication/tests/owasp_top10_2025_parser_test.pyapplication/utils/external_project_parsers/data/owasp_aisvs_1_0.jsonapplication/utils/external_project_parsers/data/owasp_api_top10_2023.jsonapplication/utils/external_project_parsers/data/owasp_llm_top10_2025.jsonapplication/utils/external_project_parsers/data/owasp_top10_2025.jsonapplication/utils/external_project_parsers/parsers/owasp_aisvs.pyapplication/utils/external_project_parsers/parsers/owasp_api_top10_2023.pyapplication/utils/external_project_parsers/parsers/owasp_llm_top10_2025.pyapplication/utils/external_project_parsers/parsers/owasp_top10_2025.pycre.py
Bornunique911
force-pushed
the
review/issue-471-ai-resource-importers-clean
branch
from
a4a595d to
4bc99ab
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application/web/web_main.py (1)
1477-1483: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winUnbounded
per_pageallows oversized pagination.The prior
min(per_page, MAX_ITEMS_PER_PAGE)cap is gone, so a caller can request an arbitrarily largeper_page, forcing the DB/serializer to materialize the entire CRE set in one request (DoS / memory pressure). Reinstate an upper bound.🛡️ Proposed fix
- if ( - request.args.get("per_page") is not None - and int(request.args.get("per_page")) > 0 - ): - per_page = int(request.args.get("per_page")) + if ( + request.args.get("per_page") is not None + and int(request.args.get("per_page")) > 0 + ): + per_page = min(int(request.args.get("per_page")), MAX_ITEMS_PER_PAGE)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/web/web_main.py` around lines 1477 - 1483, The pagination handler in application/web/web_main.py no longer caps per_page, so requests can force oversized result sets. In the logic around request.args.get("per_page") and database.all_cres_with_pagination, reinstate the existing MAX_ITEMS_PER_PAGE upper bound by clamping the parsed per_page value before passing it onward. Keep the positive-value check, but ensure the final per_page used by the pagination call cannot exceed the maximum.
🧹 Nitpick comments (4)
application/web/web_main.py (2)
411-415: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated guard.
The
if not base_nodes or not compare_nodes: return Nonecheck is repeated verbatim. Drop the second occurrence.♻️ Proposed cleanup
if not base_nodes or not compare_nodes: return None - - if not base_nodes or not compare_nodes: - return None - compare_nodes_by_cre: dict[str, list[defs.Standard]] = {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/web/web_main.py` around lines 411 - 415, The guard in web_main’s comparison logic is duplicated verbatim, so remove the repeated `if not base_nodes or not compare_nodes: return None` check and keep only one occurrence in the surrounding function to avoid redundant control flow.
293-297: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffPotential N+1 query loading all OpenCRE documents.
This issues one
get_CREs(internal_id=...)call per row returned bysession.query(db.CRE).all(). On production-sized graphs this becomes a large fan-out of queries permap_analysisrequest that hits the OpenCRE fast-path. Consider batch-loading CREs (single query) instead of per-id round trips.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/web/web_main.py` around lines 293 - 297, The _get_opencre_documents helper is doing an N+1 query pattern by calling collection.get_CREs(internal_id=cre.id) once per db.CRE row. Update this path to batch-load all needed CREs in a single query (or a small fixed number of queries) and then map them back to defs.CRE objects in memory, keeping the behavior of map_analysis/OpenCRE fast-path the same while removing the per-row round trips.application/tests/web_main_test.py (2)
1471-1471: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLeftover debug
This
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/tests/web_main_test.py` at line 1471, Remove the leftover debugging output in the test so the suite stays quiet; delete the print statement in the web_main_test flow that logs the response status and data, and keep the surrounding test logic unchanged.
1450-1451: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnv mutation leaks across tests.
Setting
os.environ["CRE_ALLOW_IMPORT"] = "True"without restoring it (the priorpatch.dictcontext handled cleanup) leaves the flag enabled for subsequent tests, which can perturb import-gating andget_configassertions depending on test ordering. Prefer@patch.dict(os.environ, {"CRE_ALLOW_IMPORT": "True"})or restore intearDown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/tests/web_main_test.py` around lines 1450 - 1451, The test in test_import_from_cre_csv mutates os.environ directly and leaves CRE_ALLOW_IMPORT enabled for later tests. Update this test to use temporary environment patching via patch.dict on os.environ (or restore the variable in tearDown) so the flag is automatically cleaned up after the test, keeping the import-gating behavior isolated for get_config and related assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@application/web/web_main.py`:
- Line 1522: `get_config` is interpreting `CRE_ALLOW_IMPORT` differently from
the import gating logic, so the config endpoint can disagree with the backend.
Update the `get_config` handling to use the same truthy membership check as the
import checks near the `CRE_ALLOW_IMPORT` gating logic and the related config
return path, so `"1"`, `"true"`, and `"yes"` are reported consistently. Keep the
behavior aligned with the existing import अनुमति/guard code rather than
comparing only against `"1"`.
- Around line 1352-1355: The prompt generation path in
prompt_client.PromptHandler currently calls generate_text without the LLM error
translation wrapper, so provider failures are not mapped correctly. Update the
web_main prompt handling flow to wrap
prompt.generate_text(message.get("prompt")) with the existing
llm_error_utils-style error translation used elsewhere, and ensure the
translated exception/response preserves provider 429 and similar LLM-specific
status codes instead of falling through as a generic 500.
- Around line 705-748: The Heroku guard in web_main’s gap analysis flow only
checks for missing standards, but it still falls through to
gap_analysis.schedule(...) on cache misses; update this branch to short-circuit
with a 404 when no cached/upstream analysis is available on Heroku. Use the
existing helpers _fetch_upstream_map_analysis and
_build_direct_cre_overlap_map_analysis in the same gap analysis path, and keep
the Heroku-specific logic in the current request handler before any job
scheduling.
---
Outside diff comments:
In `@application/web/web_main.py`:
- Around line 1477-1483: The pagination handler in application/web/web_main.py
no longer caps per_page, so requests can force oversized result sets. In the
logic around request.args.get("per_page") and database.all_cres_with_pagination,
reinstate the existing MAX_ITEMS_PER_PAGE upper bound by clamping the parsed
per_page value before passing it onward. Keep the positive-value check, but
ensure the final per_page used by the pagination call cannot exceed the maximum.
---
Nitpick comments:
In `@application/tests/web_main_test.py`:
- Line 1471: Remove the leftover debugging output in the test so the suite stays
quiet; delete the print statement in the web_main_test flow that logs the
response status and data, and keep the surrounding test logic unchanged.
- Around line 1450-1451: The test in test_import_from_cre_csv mutates os.environ
directly and leaves CRE_ALLOW_IMPORT enabled for later tests. Update this test
to use temporary environment patching via patch.dict on os.environ (or restore
the variable in tearDown) so the flag is automatically cleaned up after the
test, keeping the import-gating behavior isolated for get_config and related
assertions.
In `@application/web/web_main.py`:
- Around line 411-415: The guard in web_main’s comparison logic is duplicated
verbatim, so remove the repeated `if not base_nodes or not compare_nodes: return
None` check and keep only one occurrence in the surrounding function to avoid
redundant control flow.
- Around line 293-297: The _get_opencre_documents helper is doing an N+1 query
pattern by calling collection.get_CREs(internal_id=cre.id) once per db.CRE row.
Update this path to batch-load all needed CREs in a single query (or a small
fixed number of queries) and then map them back to defs.CRE objects in memory,
keeping the behavior of map_analysis/OpenCRE fast-path the same while removing
the per-row round trips.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 418134eb-596c-4224-a4d6-ddf0618a9861
📒 Files selected for processing (2)
application/tests/web_main_test.pyapplication/web/web_main.py
Comment on lines
705
to
748
| # On Heroku (read-only), check if standards exist before attempting Redis/queue operations | ||
| is_heroku = os.environ.get("DYNO") is not None | ||
| if is_heroku: | ||
| # Check if all requested standards exist | ||
| try: | ||
| existing_standards = database.standards() | ||
| if isinstance(existing_standards, (list, tuple, set)): | ||
| existing_lower = {str(s).lower() for s in existing_standards} | ||
| missing = [s for s in standards if str(s).lower() not in existing_lower] | ||
| if missing: | ||
| logger.info( | ||
| f"On Heroku: gap analysis request {standards_hash} references " | ||
| f"standards that do not exist: {', '.join(missing)}, returning 404" | ||
| ) | ||
| abort( | ||
| 404, f"One or more standards do not exist: {', '.join(missing)}" | ||
| ) | ||
| except Exception as exc: | ||
| # If we can't verify standards, log but don't fail (defensive) | ||
| logger.warning(f"Could not verify standards existence on Heroku: {exc}") | ||
|
|
||
| # ----- upstream: cached result ----- | ||
| cache_key = standards_hash | ||
| if database.gap_analysis_exists(cache_key): | ||
| cached = database.get_gap_analysis_result(cache_key=cache_key) | ||
| if cached: | ||
| parsed = json.loads(cached) | ||
| if "result" in parsed: | ||
| return jsonify({"result": parsed.get("result")}) | ||
|
|
||
| # Heroku serves precomputed GA from Postgres only — never Redis, RQ, or Neo4j. | ||
| if _is_heroku_deploy(): | ||
| logger.info( | ||
| "On Heroku: gap analysis cache miss for %s, returning 404", | ||
| standards_hash, | ||
| ) | ||
| abort(404, "No such Cache") | ||
|
|
||
| # If calculations are disabled, return 404 | ||
| if os.environ.get("CRE_NO_CALCULATE_GAP_ANALYSIS"): | ||
| logger.info( | ||
| f"Gap analysis calculations are disabled by CRE_NO_CALCULATE_GAP_ANALYSIS; " | ||
| f"refusing to schedule new job for {standards_hash}" | ||
| ) | ||
| upstream_gap_analysis = _fetch_upstream_map_analysis( | ||
| standards, standards_hash, database | ||
| ) | ||
| if upstream_gap_analysis: | ||
| if owasp_top10_comparison: | ||
| upstream_gap_analysis["owasp_top10_comparison"] = owasp_top10_comparison | ||
| return jsonify(upstream_gap_analysis) | ||
| direct_gap_analysis = _build_direct_cre_overlap_map_analysis( | ||
| standards, standards_hash, database | ||
| ) | ||
| if direct_gap_analysis: | ||
| if owasp_top10_comparison: | ||
| direct_gap_analysis["owasp_top10_comparison"] = owasp_top10_comparison | ||
| return jsonify(direct_gap_analysis) | ||
| if owasp_top10_comparison: | ||
| return jsonify({"owasp_top10_comparison": owasp_top10_comparison}) | ||
| abort(404, "Gap analysis calculations are disabled") |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the Heroku/production env var convention used elsewhere in the repo
rg -nP '\b(DYNO|HEROKU)\b' --type=py -C2
# Confirm what database.standards() returns (list vs other)
ast-grep run --pattern 'def standards($$$):
$$$' --lang python $(fd -t f 'db.py' application | head -1)Repository: OWASP/OpenCRE
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target slice =="
sed -n '680,770p' application/web/web_main.py
echo
echo "== search HEROKU/DYNO usage =="
rg -n --type=py '\b(HEROKU|DYNO)\b' application tests || true
echo
echo "== search gap analysis tests =="
rg -n --type=py 'gap_analysis|cache miss|HEROKU|DYNO|CRE_NO_CALCULATE_GAP_ANALYSIS' tests application || trueRepository: OWASP/OpenCRE
Length of output: 43327
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== web_main Heroku test slice =="
sed -n '1180,1245p' application/tests/web_main_test.py
echo
echo "== db.standards implementation =="
rg -n '^(\s*)def standards\(' application/database/db.py -n -C 3
sed -n '2460,2525p' application/database/db.py
echo
echo "== any env helpers for DYNO/HEROKU in tests =="
rg -n --type=py '\b(DYNO|HEROKU)\b' application/tests application | sed -n '1,120p'Repository: OWASP/OpenCRE
Length of output: 6808
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Node_collection.standards exact implementation =="
sed -n '2138,2175p' application/database/db.py
echo
echo "== Node_collection class definition context =="
rg -n '^class Node_collection|^\s+def standards\(' application/database/db.py -n -C 2
echo
echo "== any HEROKU/DYNO env setup in web tests around this case =="
sed -n '1,80p' application/tests/web_main_test.pyRepository: OWASP/OpenCRE
Length of output: 4885
Return 404 on Heroku cache misses. The Heroku branch only rejects unknown standards; if the cache lookup misses and the standards exist, this still falls through to gap_analysis.schedule(...). That breaks the cache-only production path and should short-circuit before scheduling.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 722-722: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@application/web/web_main.py` around lines 705 - 748, The Heroku guard in
web_main’s gap analysis flow only checks for missing standards, but it still
falls through to gap_analysis.schedule(...) on cache misses; update this branch
to short-circuit with a 404 when no cached/upstream analysis is available on
Heroku. Use the existing helpers _fetch_upstream_map_analysis and
_build_direct_cre_overlap_map_analysis in the same gap analysis path, and keep
the Heroku-specific logic in the current request handler before any job
scheduling.
Source: Path instructions
Comment on lines
+1352
to
+1355
| from application.prompt_client import prompt_client | ||
|
|
||
| prompt = prompt_client.PromptHandler(database) | ||
| try: | ||
| response = prompt.generate_text(message.get("prompt")) | ||
| except Exception as e: | ||
| if llm_error_utils.is_rate_limit_error(e): | ||
| return ( | ||
| jsonify( | ||
| { | ||
| "error": ( | ||
| "The AI service is temporarily rate-limited or out of quota. " | ||
| "Please try again in a minute." | ||
| ) | ||
| } | ||
| ), | ||
| 503, | ||
| ) | ||
| return ( | ||
| jsonify({"error": f"AI Service Error: {str(e)}"}), | ||
| 500, | ||
| ) | ||
| response = prompt.generate_text(message.get("prompt")) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the reported lines
sed -n '1320,1385p' application/web/web_main.py | cat -n
printf '\n--- search for llm_error_utils usage ---\n'
rg -n "llm_error_utils|PromptHandler|generate_text\(" application -S
printf '\n--- inspect llm_error_utils definition if present ---\n'
fd -a "llm_error_utils" application || true
fd -a "llm_error" application || trueRepository: OWASP/OpenCRE
Length of output: 14857
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- llm_error_utils.py ---\n'
sed -n '1,260p' application/prompt_client/llm_error_utils.py | cat -n
printf '\n--- prompt_client.py around generate_text ---\n'
sed -n '1200,1295p' application/prompt_client/prompt_client.py | cat -n
printf '\n--- any exception handling around generate_text callers in web_main ---\n'
sed -n '1348,1362p' application/web/web_main.py | cat -n
printf '\n--- tests that exercise chat completion / rate limit behavior ---\n'
sed -n '1,140p' application/tests/chat_completion_test.py | cat -n
sed -n '1,120p' application/tests/litellm_router_test.py | cat -nRepository: OWASP/OpenCRE
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- llm_error_utils.py ---'
sed -n '1,260p' application/prompt_client/llm_error_utils.py | cat -n
printf '%s\n' ''
printf '%s\n' '--- prompt_client.py around generate_text ---'
sed -n '1200,1295p' application/prompt_client/prompt_client.py | cat -n
printf '%s\n' ''
printf '%s\n' '--- web_main.py around chat_cre ---'
sed -n '1348,1362p' application/web/web_main.py | cat -n
printf '%s\n' ''
printf '%s\n' '--- chat_completion_test.py ---'
sed -n '1,140p' application/tests/chat_completion_test.py | cat -n
printf '%s\n' ''
printf '%s\n' '--- litellm_router_test.py ---'
sed -n '1,120p' application/tests/litellm_router_test.py | cat -nRepository: OWASP/OpenCRE
Length of output: 11657
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- error handlers and genai references in web_main.py ---'
rg -n "errorhandler|genai|AI Service Error|rate-limited|is_rate_limit_error|abort\\(" application/web/web_main.py application -S
printf '%s\n' ''
printf '%s\n' '--- relevant slices in web_main.py around error handlers ---'
# show only the parts around any error handlers if present
awk 'NR>=1 && NR<=2600 {print NR ":" $0}' application/web/web_main.py | \
rg -n "errorhandler|genai|AI Service Error|rate-limited|is_rate_limit_error|abort\\(" -A6 -B6
printf '%s\n' ''
printf '%s\n' '--- search for chat completion route tests references ---'
rg -n "completion|AI Service Error|rate-limited|ClientError|is_rate_limit_error" application/tests application -SRepository: OWASP/OpenCRE
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- error handlers and genai references in web_main.py ---'
rg -n "errorhandler|genai|AI Service Error|rate-limited|is_rate_limit_error|abort\\(" application/web/web_main.py application -S
printf '%s\n' ''
printf '%s\n' '--- relevant slices in web_main.py around error handlers ---'
python3 - <<'PY'
from pathlib import Path
text = Path('application/web/web_main.py').read_text().splitlines()
keys = ['errorhandler', 'genai', 'AI Service Error', 'rate-limited', 'is_rate_limit_error', 'abort(']
for i, line in enumerate(text, 1):
if any(k in line for k in keys):
start = max(1, i-6)
end = min(len(text), i+6)
print(f'@@ around line {i} @@')
for j in range(start, end+1):
print(f'{j}:{text[j-1]}')
print()
PY
printf '%s\n' ''
printf '%s\n' '--- search for chat completion route tests references ---'
rg -n "completion|AI Service Error|rate-limited|ClientError|is_rate_limit_error" application/tests application -SRepository: OWASP/OpenCRE
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- prompt_client.py tail of generate_text ---'
python3 - <<'PY'
from pathlib import Path
text = Path('application/prompt_client/prompt_client.py').read_text().splitlines()
for start in [1290, 1330, 1370]:
pass
# print a focused window around the generate_text method tail
for i in range(1235, 1325):
if i <= len(text):
print(f"{i}:{text[i-1]}")
PY
printf '%s\n' ''
printf '%s\n' '--- web_main.py genai / rate-limit handling slices ---'
python3 - <<'PY'
from pathlib import Path
text = Path('application/web/web_main.py').read_text().splitlines()
keys = ['genai', 'rate-limited', 'AI Service Error', 'is_rate_limit_error', 'ClientError']
for i, line in enumerate(text, 1):
if any(k in line for k in keys):
start = max(1, i-4)
end = min(len(text), i+6)
print(f'@@ around line {i} @@')
for j in range(start, end+1):
print(f'{j}:{text[j-1]}')
print()
PYRepository: OWASP/OpenCRE
Length of output: 4171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- prompt_client.py tail of generate_text ---'
python3 - <<'PY'
from pathlib import Path
text = Path('application/prompt_client/prompt_client.py').read_text().splitlines()
for i in range(1235, 1325):
if i <= len(text):
print(f"{i}:{text[i-1]}")
PY
printf '%s\n' ''
printf '%s\n' '--- web_main.py genai / rate-limit handling slices ---'
python3 - <<'PY'
from pathlib import Path
text = Path('application/web/web_main.py').read_text().splitlines()
keys = ['genai', 'rate-limited', 'AI Service Error', 'is_rate_limit_error', 'ClientError']
for i, line in enumerate(text, 1):
if any(k in line for k in keys):
start = max(1, i-4)
end = min(len(text), i+6)
print(f'@@ around line {i} @@')
for j in range(start, end+1):
print(f'{j}:{text[j-1]}')
print()
PYRepository: OWASP/OpenCRE
Length of output: 4171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- search for the error translation handler and messages ---'
rg -n "AI Service Error|rate-limited|is_rate_limit_error|ClientError|genai_errors|genai" application -S --glob '!application/static/**' --glob '!**/*.min.js'
printf '%s\n' ''
printf '%s\n' '--- potential Flask error handlers ---'
rg -n "`@app`\.errorhandler|@.*errorhandler|register_error_handler|handle_exception|handle_user_exception" application -S --glob '!application/static/**'
printf '%s\n' ''
printf '%s\n' '--- chat completion route tests references ---'
rg -n "completion|AI Service Error|rate-limited|ClientError" application/tests -SRepository: OWASP/OpenCRE
Length of output: 3954
Restore LLM error translation here
prompt.generate_text(message.get("prompt")) now runs with no llm_error_utils-style wrapper, so provider 429s and other LLM failures fall through as generic 500s instead of mapped response codes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@application/web/web_main.py` around lines 1352 - 1355, The prompt generation
path in prompt_client.PromptHandler currently calls generate_text without the
LLM error translation wrapper, so provider failures are not mapped correctly.
Update the web_main prompt handling flow to wrap
prompt.generate_text(message.get("prompt")) with the existing
llm_error_utils-style error translation used elsewhere, and ensure the
translated exception/response preserves provider 429 and similar LLM-specific
status codes instead of falling through as a generic 500.
| @app.route("/rest/v1/config", methods=["GET"]) | ||
| def get_config() -> Any: | ||
| return jsonify({"CRE_ALLOW_IMPORT": is_cre_import_allowed()}) | ||
| return jsonify({"CRE_ALLOW_IMPORT": os.environ.get("CRE_ALLOW_IMPORT") == "1"}) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Inconsistent CRE_ALLOW_IMPORT interpretation.
get_config reports import enabled only when the value is exactly "1", but the import gating at Lines 1156-1160 and Line 1554 accepts 1/true/yes. With CRE_ALLOW_IMPORT="true" the backend allows imports while /rest/v1/config reports them disabled — the UI and backend disagree. Align both on the same membership check.
🔧 Proposed fix
- return jsonify({"CRE_ALLOW_IMPORT": os.environ.get("CRE_ALLOW_IMPORT") == "1"})
+ return jsonify(
+ {
+ "CRE_ALLOW_IMPORT": str(os.environ.get("CRE_ALLOW_IMPORT", "")).lower()
+ in ("1", "true", "yes")
+ }
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return jsonify({"CRE_ALLOW_IMPORT": os.environ.get("CRE_ALLOW_IMPORT") == "1"}) | |
| return jsonify( | |
| { | |
| "CRE_ALLOW_IMPORT": str(os.environ.get("CRE_ALLOW_IMPORT", "")).lower() | |
| in ("1", "true", "yes") | |
| } | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@application/web/web_main.py` at line 1522, `get_config` is interpreting
`CRE_ALLOW_IMPORT` differently from the import gating logic, so the config
endpoint can disagree with the backend. Update the `get_config` handling to use
the same truthy membership check as the import checks near the
`CRE_ALLOW_IMPORT` gating logic and the related config return path, so `"1"`,
`"true"`, and `"yes"` are reported consistently. Keep the behavior aligned with
the existing import अनुमति/guard code rather than comparing only against `"1"`.
1e4de18 to
090da0b
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #858. This replacement PR keeps only the AI/API/LLM/Top10 parser modules, mapping data, parser tests, and the minimal CLI wiring needed for standalone review from main.