Normalize OWASP cheat sheet references - #954
Open
Bornunique911 wants to merge 5 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThis PR updates the OWASP cheat sheet parser to combine repository entries with supplemental metadata. It generates official OWASP HTML URLs, handles clone failures, creates CRE links, and merges duplicate entries. ChangesCheat sheet registration and supplemental linking
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Bornunique911
force-pushed
the
review/issue-471-cheatsheet-references-clean
branch
from
July 7, 2026 12:23
7f640f4 to
aac9e3b
Compare
Bornunique911
force-pushed
the
review/issue-471-cheatsheet-references-clean
branch
from
July 26, 2026 13:14
aac9e3b to
79cab23
Compare
Bornunique911
requested review from
Pa04rth,
northdpole,
paoga87 and
robvanderveer
as code owners
July 26, 2026 13:14
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
application/tests/cheatsheets_parser_test.py (1)
81-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover clone fallback and duplicate merging.
The new test covers supplemental loading only. Add cases where
git.clonefails but a seeded supplemental entry is still returned, and where repository/supplement entries share(section, hyperlink)and their CRE links are merged. This protects both new parse paths.🤖 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/cheatsheets_parser_test.py` around lines 81 - 102, Add test cases alongside test_register_supplemental_cheatsheets covering clone failure with a seeded supplemental entry still returned, and repository/supplement entries sharing the same (section, hyperlink) with their CRE links merged. Configure the git.clone failure through the existing test seam and assert both fallback retention and deduplicated combined links.
🤖 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/utils/external_project_parsers/parsers/cheatsheets_parser.py`:
- Around line 57-61: Restrict the exception handler around the OWASP
CheatSheetSeries clone in the parser to expected process and OS failures raised
by git.clone(), such as subprocess and operating-system errors. Do not catch
arbitrary Exception, so parser and configuration defects propagate normally
while the existing warning and supplemental-cheat-sheets fallback remain
unchanged for those failures.
---
Nitpick comments:
In `@application/tests/cheatsheets_parser_test.py`:
- Around line 81-102: Add test cases alongside
test_register_supplemental_cheatsheets covering clone failure with a seeded
supplemental entry still returned, and repository/supplement entries sharing the
same (section, hyperlink) with their CRE links merged. Configure the git.clone
failure through the existing test seam and assert both fallback retention and
deduplicated combined links.
🪄 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 Plus
Run ID: d61289eb-2550-49a5-9d56-a8af8cd692f9
📒 Files selected for processing (3)
application/tests/cheatsheets_parser_test.pyapplication/utils/external_project_parsers/data/owasp_cheatsheets_supplement.jsonapplication/utils/external_project_parsers/parsers/cheatsheets_parser.py
Bornunique911
force-pushed
the
review/issue-471-cheatsheet-references-clean
branch
from
July 31, 2026 19:21
cc87a19 to
5d5b850
Compare
Bornunique911
force-pushed
the
review/issue-471-cheatsheet-references-clean
branch
from
August 4, 2026 16:36
5d5b850 to
f8912f9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
application/tests/cheatsheets_parser_test.py (2)
93-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
next(...)for single-match lookups.These comprehensions allocate a list only to retrieve its first item. Use
next(...)to resolve Ruff RUF015.Proposed change
- rest = [ - entry for entry in entries if entry.section == "REST Security Cheat Sheet" - ][0] + rest = next( + entry + for entry in entries + if entry.section == "REST Security Cheat Sheet" + ) ... - rest = [ - node - for node in entries.results["OWASP Cheat Sheets"] - if node.section == "REST Security Cheat Sheet" - ][0] + rest = next( + node + for node in entries.results["OWASP Cheat Sheets"] + if node.section == "REST Security Cheat Sheet" + )Also applies to: 125-129
🤖 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/cheatsheets_parser_test.py` around lines 93 - 95, Update the single-match lookups in the test around the `rest` assignment and the additional occurrence at the referenced location to use `next(...)` over the filtered generator instead of building a list and indexing `[0]`, resolving Ruff RUF015 while preserving the existing match behavior.Source: Linters/SAST tools
100-103: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the supplemental link type.
register_supplemental_cheatsheetsinapplication/utils/external_project_parsers/parsers/cheatsheets_parser.py(Lines 108-140) createsAutomaticallyLinkedTolinks. This test only checks document IDs, so a different link type would pass.Proposed change
self.assertEqual( ["118-110", "724-770", "623-550"], [link.document.id for link in rest.links], ) + self.assertEqual( + [defs.LinkTypes.AutomaticallyLinkedTo] * 3, + [link.ltype for link in rest.links], + )🤖 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/cheatsheets_parser_test.py` around lines 100 - 103, The test assertion at lines 100-103 only validates the document IDs from rest.links but does not verify the link type. Add an assertion that checks each link in rest.links is of the expected type (AutomaticallyLinkedTo) to ensure the register_supplemental_cheatsheets parser is creating the correct link type, not just returning the correct document IDs.
🤖 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/tests/cheatsheets_parser_test.py`:
- Line 149: Replace the tempfile.mkdtemp() usage in the affected test with a
tempfile.TemporaryDirectory context or managed instance, ensuring the temporary
repository directory is automatically cleaned up after the test while preserving
the existing loc path usage.
In `@application/utils/external_project_parsers/parsers/cheatsheets_parser.py`:
- Around line 130-139: The link-building handler around cre.shallow_copy() and
cs.add_link() must not suppress unexpected programming errors. Restrict the
except block to the specific expected link-validation exception(s), allowing all
other exceptions to propagate, while preserving the existing warning and
add_link_failures behavior for expected failures.
---
Nitpick comments:
In `@application/tests/cheatsheets_parser_test.py`:
- Around line 93-95: Update the single-match lookups in the test around the
`rest` assignment and the additional occurrence at the referenced location to
use `next(...)` over the filtered generator instead of building a list and
indexing `[0]`, resolving Ruff RUF015 while preserving the existing match
behavior.
- Around line 100-103: The test assertion at lines 100-103 only validates the
document IDs from rest.links but does not verify the link type. Add an assertion
that checks each link in rest.links is of the expected type
(AutomaticallyLinkedTo) to ensure the register_supplemental_cheatsheets parser
is creating the correct link type, not just returning the correct document IDs.
🪄 Autofix
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 Plus
Run ID: 8aa1024b-27a6-463c-9173-f1f6a971f6cf
📒 Files selected for processing (3)
application/tests/cheatsheets_parser_test.pyapplication/utils/external_project_parsers/data/owasp_cheatsheets_supplement.jsonapplication/utils/external_project_parsers/parsers/cheatsheets_parser.py
🚧 Files skipped from review as they are similar to previous changes (1)
- application/utils/external_project_parsers/data/owasp_cheatsheets_supplement.json
…heatsheets parser - Replace `tempfile.mkdtemp()` with `tempfile.TemporaryDirectory` + `addCleanup` in `test_parse_merges_repo_and_supplemental_duplicate_entries` to ensure automatic cleanup of temporary repository directory. - Narrow exception handling in `register_supplemental_cheatsheets` to catch only `ValueError` for expected link‑validation errors, allowing unexpected programming errors to propagate. - Convert list‑index lookups (`[...][0]`) to `next()` over generators in `test_register_supplemental_cheatsheets` and `test_parse_returns_supplemental_entries_when_clone_fails` (RUF015). - Add assertion in `test_register_supplemental_cheatsheets` to verify that all links are of type `AutomaticallyLinkedTo`, ensuring correct link type creation.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
application/tests/cheatsheets_parser_test.py (2)
118-125: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify that the clone-failure path runs.
mock_clone.side_effectonly affects the test whengit.cloneis called. Without a call assertion,parse()could skip cloning and still return the supplemental entry. Addmock_clone.assert_called()afterparse().Proposed test assertion
entries = cheatsheets_parser.Cheatsheets().parse( cache=self.collection, ph=PromptHandler(database=self.collection) ) + mock_clone.assert_called()🤖 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/cheatsheets_parser_test.py` around lines 118 - 125, Add a call assertion immediately after the Cheatsheets().parse invocation to verify mock_clone was called, ensuring the test exercises the clone-failure path rather than passing without cloning.
67-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a non-empty parser result.
If
entries.resultsis empty, the loop at Line 67 does not execute. The test then passes without checking the registration. Assert thatparser.nameis present and inspect that result directly.Proposed test assertion
- for name, nodes in entries.results.items(): - self.assertEqual(name, parser.name) + self.assertIn(parser.name, entries.results) + nodes = entries.results[parser.name]🤖 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/cheatsheets_parser_test.py` around lines 67 - 80, Update the test around parser.name and entries.results to assert that parser.name is present before inspecting its nodes, rather than relying on the for loop to execute. Retrieve the result for parser.name directly and keep the existing section and expected secret_entry assertions against that result, ensuring an empty registration fails the test.
🤖 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.
Outside diff comments:
In `@application/tests/cheatsheets_parser_test.py`:
- Around line 118-125: Add a call assertion immediately after the
Cheatsheets().parse invocation to verify mock_clone was called, ensuring the
test exercises the clone-failure path rather than passing without cloning.
- Around line 67-80: Update the test around parser.name and entries.results to
assert that parser.name is present before inspecting its nodes, rather than
relying on the for loop to execute. Retrieve the result for parser.name directly
and keep the existing section and expected secret_entry assertions against that
result, ensuring an empty registration fails the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3198380c-4c17-4ae3-8514-19c44b679c0c
📒 Files selected for processing (2)
application/tests/cheatsheets_parser_test.pyapplication/utils/external_project_parsers/parsers/cheatsheets_parser.py
🚧 Files skipped from review as they are similar to previous changes (1)
- application/utils/external_project_parsers/parsers/cheatsheets_parser.py
- Replace `tempfile.mkdtemp()` with `tempfile.TemporaryDirectory` + `addCleanup` in `test_parse_merges_repo_and_supplemental_duplicate_entries` to ensure automatic cleanup of temporary repository directory. - Narrow exception handling in `register_supplemental_cheatsheets` to catch only `ValueError` for expected link‑validation errors, allowing unexpected programming errors to propagate. - Convert list‑index lookups (`[...][0]`) to `next()` over generators in `test_register_supplemental_cheatsheets` and `test_parse_returns_supplemental_entries_when_clone_fails` (RUF015). - Add assertion in `test_register_supplemental_cheatsheets` to verify that all links are of type `AutomaticallyLinkedTo`, ensuring correct link type creation. - In `test_parse_returns_supplemental_entries_when_clone_fails`, add `mock_clone.assert_called_once()` to verify the clone-failure path is exercised. - In `test_register_cheatsheet`, replace the `for`-loop over `entries.results` with a direct lookup of `parser.name`, ensuring the test fails explicitly when `entries.results` is empty rather than silently passing.
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.
Summary
This PR is split out from the larger issue-471 review flow to make review smaller and more focused.
It normalizes OWASP cheat sheet references by:
Issue reference:
Problem Fixed
The earlier cheat sheet references review became too large because it was mixed with unrelated importer-stack work.
For this part of the work, the useful standalone contribution is:
Solution
This PR updates the cheat sheet parser to:
This PR also adds:
Files in scope:
application/utils/external_project_parsers/parsers/cheatsheets_parser.pyapplication/utils/external_project_parsers/data/owasp_cheatsheets_supplement.jsonapplication/tests/cheatsheets_parser_test.pyTests
Reviewer Notes
This PR is intentionally narrow because it was split to reduce review size:
This PR is meant to be reviewed as a standalone cheat sheet reference normalization change.