docs: clarify local contributor setup for upstream sync and MyOpenCRE - #997
docs: clarify local contributor setup for upstream sync and MyOpenCRE#997Bornunique911 wants to merge 1 commit into
Conversation
WalkthroughChangesLocal development workflow
Estimated code review effort: 2 (Simple) | ~10 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@README.md`:
- Around line 138-142: Document that POST /rest/v1/cre_csv_import requires the
multipart file field named cre_csv, or add a request example showing that field,
in both README.md lines 138-142 and docs/developmentSetup.md lines 139-143.
- Around line 129-136: Remove the premature Flask startup from the optional
import instructions: in README.md lines 129-136, retain only the
CRE_ALLOW_IMPORT=true setup and rely on the later local-running startup block;
in docs/developmentSetup.md lines 131-137, remove or relocate make dev-flask so
Flask is started only once after the required setup.
- Around line 117-126: Remove the documented `python cre.py --review
--from_spreadsheet` command from the README, since `--review` is not an exposed
CLI argument; retain the valid `--add --from_spreadsheet` example.
🪄 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: 1c5b47f8-eefd-4587-bb44-0fdd49f1d084
📒 Files selected for processing (2)
README.mddocs/developmentSetup.md
3006cae to
e806905
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
application/tests/cwe_parser_test.py (2)
299-308: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant re-add/commit of an already persisted node.
Node_collection.add_nodeinserts and commits, returning a persistentNode; re-adding it to the session and committing again is a no-op that obscures the setup intent.♻️ Proposed cleanup
- stale_category = self.collection.add_node( + self.collection.add_node( defs.Standard( name="CWE", sectionID="16", section="Configuration", hyperlink="https://cwe.mitre.org/data/definitions/16.html", ) ) - self.collection.session.add(stale_category) - self.collection.session.commit()🤖 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/cwe_parser_test.py` around lines 299 - 308, Remove the redundant session.add and session.commit calls after collection.add_node in the stale_category setup. Keep the returned persistent node from Node_collection.add_node unchanged and preserve the existing test data.
812-826: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixture only exercises the
Mapping_Notes/Usagebranch.Both prohibited entries rely on mapping usage;
is_prohibited_entry's@Status == "prohibited"branch is untested. Consider adding one weakness withStatus="Prohibited"(noMapping_Notes) to cover it, and runmake testto confirm.As per coding guidelines: "Run
make testto execute the Python unittest suite and verify behavior".🤖 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/cwe_parser_test.py` around lines 812 - 826, Extend the XML fixture used by the parser tests to include a weakness with Status="Prohibited" and no Mapping_Notes, so is_prohibited_entry covers its status-based branch in addition to the existing usage-based cases. Run make test to verify the unittest suite.Source: Coding guidelines
application/utils/external_project_parsers/parsers/cwe.py (2)
299-303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
idto avoid shadowing the builtin (Ruff A001).♻️ Proposed fix
else: - id = lst["`@CAPEC_ID`"] - cwe = self.link_cwe_to_capec_cre( - cwe=cwe, cache=cache, capec_id=id - ) + capec_id = lst["`@CAPEC_ID`"] + cwe = self.link_cwe_to_capec_cre( + cwe=cwe, cache=cache, capec_id=capec_id + )🤖 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/cwe.py` around lines 299 - 303, In the CAPEC linking branch, rename the local variable id to a non-conflicting name and update the capec_id argument in self.link_cwe_to_capec_cre accordingly, preserving the existing value from lst["`@CAPEC_ID`"].Source: Linters/SAST tools
74-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
dictovertyping.Dictinisinstancechecks.
isinstance(x, Dict)works at runtime but is non-idiomatic and mypy flags parameterized generics here inconsistently; plaindictis clearer and also matchesOrderedDictreturned byxmltodict.♻️ Suggested tweak
- if isinstance(collection, list): - entries.extend(entry for entry in collection if isinstance(entry, Dict)) - elif isinstance(collection, Dict): + if isinstance(collection, list): + entries.extend(entry for entry in collection if isinstance(entry, dict)) + elif isinstance(collection, dict): entries.append(collection)- if not isinstance(mapping_notes, Dict): + if not isinstance(mapping_notes, dict): return ""🤖 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/cwe.py` around lines 74 - 105, Update the isinstance checks in iter_catalog_entries to use the built-in dict type instead of typing.Dict, including both collection and entry checks. Keep the existing list/dict traversal and filtering behavior unchanged.
🤖 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 `@README.md`:
- Around line 120-143: Update the CSV import workflow documentation near the
CRE_ALLOW_IMPORT and make start-containers instructions to explicitly start the
Flask API with make dev-flask or make dev-flask-docker before calling the
/rest/v1/cre_csv and /rest/v1/cre_csv_import endpoints.
---
Nitpick comments:
In `@application/tests/cwe_parser_test.py`:
- Around line 299-308: Remove the redundant session.add and session.commit calls
after collection.add_node in the stale_category setup. Keep the returned
persistent node from Node_collection.add_node unchanged and preserve the
existing test data.
- Around line 812-826: Extend the XML fixture used by the parser tests to
include a weakness with Status="Prohibited" and no Mapping_Notes, so
is_prohibited_entry covers its status-based branch in addition to the existing
usage-based cases. Run make test to verify the unittest suite.
In `@application/utils/external_project_parsers/parsers/cwe.py`:
- Around line 299-303: In the CAPEC linking branch, rename the local variable id
to a non-conflicting name and update the capec_id argument in
self.link_cwe_to_capec_cre accordingly, preserving the existing value from
lst["`@CAPEC_ID`"].
- Around line 74-105: Update the isinstance checks in iter_catalog_entries to
use the built-in dict type instead of typing.Dict, including both collection and
entry checks. Keep the existing list/dict traversal and filtering behavior
unchanged.
🪄 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: 92306be1-1c77-4a4a-a612-efa8d4cded5a
📒 Files selected for processing (5)
README.mdapplication/tests/cwe_parser_test.pyapplication/utils/external_project_parsers/parsers/cwe.pydocs/developmentSetup.mdscripts/update-cwe.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/developmentSetup.md
374d3a9 to
f3d9fc0
Compare
747dcb8 to
f3d9fc0
Compare
f3d9fc0 to
953d042
Compare
Summary
This PR updates the contributor documentation for issue #599 to clarify the supported local development workflow for OpenCRE.
In particular, it makes it explicit that contributors do not need access to the internal OpenCRE Google Sheet for normal local development, and that the supported bootstrap path is to create the local schema, sync from upstream, and optionally use local MyOpenCRE / CSV import when needed.
Problem
The current setup documentation leaves a gap around how contributors should work locally when they do not have access to the internal OpenCRE spreadsheet workflow.
Because of that, it is easy to assume that Google Sheet access is required to get started, even though the supported contributor path is actually:
That documentation gap is what issue
#599is about.Solution
This PR clarifies the supported contributor workflow in the docs by:
make migrate-upgradefollowed bymake upstream-syncas the standard local bootstrap pathmake upstream-syncpulls the public OpenCRE graph into the local databaseCRE_ALLOW_IMPORT=true, MyOpenCRE, and the CSV import endpointsChanged Files
Changed files:
README.mddocs/developmentSetup.mdThis is a docs-only change. It does not modify runtime behavior, import logic, or database code.