docs: clarify local contributor setup for upstream sync and MyOpenCRE - #997
Open
Bornunique911 wants to merge 1 commit into
Open
docs: clarify local contributor setup for upstream sync and MyOpenCRE#997Bornunique911 wants to merge 1 commit into
Bornunique911 wants to merge 1 commit into
Conversation
Bornunique911
requested review from
Pa04rth,
northdpole,
paoga87 and
robvanderveer
as code owners
July 28, 2026 13:57
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe PR updates local development documentation to run schema migrations before upstream synchronization, clarifies that Google Sheet access is unnecessary, and documents optional CSV mapping imports through local REST endpoints. ChangesLocal development workflow
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.
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
Bornunique911
force-pushed
the
docs/issue-599-local-setup-clarity
branch
3 times, most recently
from
July 28, 2026 15:05
3006cae to
e806905
Compare
Contributor
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
Bornunique911
force-pushed
the
docs/issue-599-local-setup-clarity
branch
3 times, most recently
from
July 28, 2026 15:23
374d3a9 to
f3d9fc0
Compare
Bornunique911
marked this pull request as draft
July 28, 2026 15:25
Bornunique911
force-pushed
the
docs/issue-599-local-setup-clarity
branch
2 times, most recently
from
July 28, 2026 15:30
f3d9fc0 to
953d042
Compare
Bornunique911
marked this pull request as ready for review
July 28, 2026 15:32
Bornunique911
force-pushed
the
docs/issue-599-local-setup-clarity
branch
3 times, most recently
from
July 31, 2026 18:11
afcc0ff to
a42f560
Compare
Bornunique911
force-pushed
the
docs/issue-599-local-setup-clarity
branch
from
July 31, 2026 18:12
a42f560 to
1da638a
Compare
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 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.