fix: address full-codebase review findings (bugs, docs, tests, CI) - #32
Open
japrescott wants to merge 1 commit into
Open
fix: address full-codebase review findings (bugs, docs, tests, CI)#32japrescott wants to merge 1 commit into
japrescott wants to merge 1 commit into
Conversation
Remediates the findings in REVIEW.md across bugs, docs, tests, and CI: - Bugs: map x-api-key in .mcp.json; set exec bit + harden poll_batch_job.py (API retry, distinct exit code, interval validation); replace fake rf.universe() SDK call; fix broken/contradictory training tables; make inference.md prefer workflows_run; fail installers loudly instead of no-op. - Docs: strip skill frontmatter from sub-pages; convert roboflow:// cross-refs to relative links; de-risk hard-coded pricing; assorted nits + Codex stamp. - Tests/CI: add pytest suite for the poller and a validate.yml workflow (JSON, links, frontmatter, exec-bit, ruff/py_compile, manifest versions); add SECURITY.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR remediates the repository-wide issues documented in REVIEW.md by tightening MCP auth configuration, hardening the batch-job polling helper, converting cross-skill references to relative links, and adding CI + tests to prevent regressions.
Changes:
- Add a validation workflow plus a docs validator script (links/frontmatter), Python lint/compile, exec-bit checks, and a pytest suite for the batch poller.
- Harden
skills/inference/bin/poll_batch_job.py(argument validation, retry behavior, distinct exit codes) and document the pinnedinference-clirange. - Normalize skill documentation: remove sub-page frontmatter, replace
roboflow://skills/...references with relative links, and correct several doc inaccuracies.
Reviewed changes
Copilot reviewed 25 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_poll_batch_job.py | Adds unit tests for the poller script via module stubbing. |
| skills/inference/bin/poll_batch_job.py | Hardens polling behavior, argument validation, and exit-code semantics. |
| skills/inference/batch-jobs.md | Updates poller usage docs (pinned CLI range, exit code 3). |
| .github/workflows/validate.yml | Adds CI to validate JSON, docs structure, Python lint/compile, exec bits, and tests. |
| scripts/validate_docs.py | Adds repo-local docs validation for links and frontmatter rules. |
| .mcp.json | Maps ${ROBOFLOW_API_KEY} into x-api-key for MCP auth. |
| README.md | Clarifies MCP auth behavior and adds a Codex doc freshness note. |
| agent-install/agent.sh | Makes placeholder installer fail loudly (non-zero). |
| agent-install/agent.ps1 | Makes placeholder installer fail loudly (non-zero). |
| SECURITY.md | Adds a security reporting policy stub for the repo. |
| REVIEW.md | Adds the documented review findings this PR is addressing. |
| .gitignore | Ignores Python caches for new tooling/tests. |
| skills/training-and-evaluation/SKILL.md | Fixes/aligns architecture tables and converts related links to relative. |
| skills/training-and-evaluation/improvement-playbook.md | Removes frontmatter and converts cross-links to relative. |
| skills/training-and-evaluation/active-learning.md | Removes frontmatter and converts cross-links to relative. |
| skills/inference/SKILL.md | Replaces MCP URI references with relative links. |
| skills/inference/workflows.md | Clarifies SAM3 wording in the block table. |
| skills/api-reference/SKILL.md | Converts related-page links to relative. |
| skills/api-reference/inference.md | Rewords guidance to prefer workflows_run; adds 401/403 host note and relative links. |
| skills/api-reference/api-key-management.md | Fixes “three vs four states of scopes” wording. |
| skills/product-navigation/SKILL.md | Converts related-page link to relative. |
| skills/product-navigation/features-by-page.md | Fixes a non-existent SDK call and converts cross-skill links to relative. |
| skills/plans-and-pricing/SKILL.md | Removes hard-coded dollar amounts in favor of pointing to pricing page. |
| skills/data-management/SKILL.md | Converts related links to relative. |
| skills/data-management/labeling.md | Removes sub-page frontmatter. |
| skills/cloud-storage/SKILL.md | Converts related links to relative. |
| skills/universe/SKILL.md | Converts related links to relative. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+47
to
+50
| _spec = importlib.util.spec_from_file_location("poll_batch_job", _MODULE_PATH) | ||
| poll = importlib.util.module_from_spec(_spec) | ||
| _spec.loader.exec_module(poll) | ||
|
|
Comment on lines
+30
to
+32
| - name: Install tools | ||
| run: pip install --quiet ruff pytest | ||
|
|
Comment on lines
98
to
101
| | Fork dataset to workspace | `universe.roboflow.com/{user}/{proj}` -> Fork | -- | | ||
| | Try a pretrained model | `universe.roboflow.com/{user}/{proj}/model` | MCP: `models_infer` with universe model ID | | ||
| | Download dataset | `universe.roboflow.com/{user}/{proj}` -> Download | Python SDK: `rf.universe(user, proj).version(v).download(fmt)` | | ||
| | Download dataset | `universe.roboflow.com/{user}/{proj}` -> Download | Python SDK: `rf.workspace(user).project(proj).version(v).download(fmt)` (or copy the snippet Universe shows on the Download tab) | | ||
|
|
Member
|
Seems similar to #27 |
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
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.
Remediates the findings documented in
REVIEW.md. Bug fixes: the bundled.mcp.jsonnow maps${ROBOFLOW_API_KEY}into thex-api-keyheader,poll_batch_job.pygets its exec bit plus API-retry/exit-code/interval hardening, the non-existentrf.universe()call is replaced, the broken/contradictory training-and-evaluation tables and YOLO-NAS sizes are corrected,inference.mdnow prefersworkflows_runover the inline-spec exception, and the placeholder installers fail loudly instead of exiting 0. Docs: skill-style frontmatter is stripped from sub-pages, allroboflow://cross-references become relative links, hard-coded dollar prices are removed in favor of pointing at roboflow.com/pricing, and assorted nits (scopes count, SAM3 phrasing, 401/403 host note, Codex date-stamp) are fixed. Tooling: adds a pytest suite for the poller and avalidate.ymlCI workflow (JSON lint, relative-link + frontmatter checks, exec-bit/shebang, ruff/py_compile, and plugin-manifest version equality) plus aSECURITY.mdstub. A few review items are intentionally left for a follow-up decision (skill-name frontmatter rename, MCP OAuth-vs-header client verification, the staging base URL); see the flagged section inREVIEW.md.🤖 Generated with Claude Code