Closed
Conversation
📝 WalkthroughWalkthroughAdds a new developer guide at Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/copilot-instructions.md (1)
128-137: Document the E2E fixture safety pattern here as well.Please add a short note that read-only/safe mutations should reuse existing
e2e_configresources, while destructive tests should use isolated create/cleanup fixtures.Suggested addition
### Fixtures Common fixtures are in `tests/unit/conftest.py`: - `http_client` — `HTTPClient` instance - `async_http_client` — `AsyncHTTPClient` instance + +For E2E tests, prefer reusing `e2e_config` resources for read-only and safe mutations. +Use per-test created fixtures only for destructive/dangerous scenarios.Based on learnings: In the mpt-api-python-client E2E tests, the team intentionally reuses existing API resources (via e2e_config) for read-only operations and safe mutations, while destructive tests use isolated created fixtures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/copilot-instructions.md around lines 128 - 137, Add a short E2E fixture safety note to the fixtures section explaining that tests should reuse existing e2e resources for read-only or non-destructive/safe mutations by using the e2e_config fixture, while destructive tests must create isolated resources and clean them up via dedicated create/cleanup fixtures; mention the pattern used in mpt-api-python-client E2E tests (reuse via e2e_config vs isolated create/cleanup) and give a one-line example guideline for each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/copilot-instructions.md:
- Around line 36-41: The fenced code block that starts with "MPTClient /
AsyncMPTClient" is unlabeled and triggers markdownlint MD040; add a language
hint by replacing the opening triple backticks with a labeled fence (for example
use "text") so the block becomes ```text followed by the existing lines and
closed with ```, ensuring the block content ("MPTClient / AsyncMPTClient ...
HTTPClient / AsyncHTTPClient (wraps httpx)") remains unchanged.
In `@Dockerfile`:
- Around line 19-20: Replace the unsafe "curl -fsSL
https://gh.io/copilot-install | bash" RUN step with a pinned, verified install:
install curl and ca-certificates via apt-get with --no-install-recommends and
clean apt lists, then either invoke the official installer while setting a
pinned VERSION environment variable (e.g. VERSION="vX.Y.Z") so the installer
performs SHA256 verification, or download the specific release artifacts
(copilot tarball and SHA256SUMS.txt), verify the checksum with sha256sum -c, and
install the binary to /usr/local/bin; update the Dockerfile RUN that currently
contains "curl -fsSL https://gh.io/copilot-install | bash" accordingly and
ensure the chosen VERSION value is explicit and checked.
---
Nitpick comments:
In @.github/copilot-instructions.md:
- Around line 128-137: Add a short E2E fixture safety note to the fixtures
section explaining that tests should reuse existing e2e resources for read-only
or non-destructive/safe mutations by using the e2e_config fixture, while
destructive tests must create isolated resources and clean them up via dedicated
create/cleanup fixtures; mention the pattern used in mpt-api-python-client E2E
tests (reuse via e2e_config vs isolated create/cleanup) and give a one-line
example guideline for each case.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 197d7428-5a06-4357-88a9-dc813cc954e5
📒 Files selected for processing (2)
.github/copilot-instructions.mdDockerfile
robcsegal
approved these changes
Mar 13, 2026
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Dockerfile (1)
20-20:⚠️ Potential issue | 🟠 MajorUnpinned remote installer is still unresolved from the previous review.
Line 20 still executes
curl ... | bashas root without an explicit pinned version/checksum flow, so reproducibility and supply-chain guarantees remain weak.Suggested hardening (keeps dev-stage scope)
+ARG COPILOT_VERSION=v0.0.369 -RUN curl -fsSL https://gh.io/copilot-install | bash +RUN curl -fsSL https://gh.io/copilot-install | VERSION="${COPILOT_VERSION}" PREFIX="/usr/local" bashWhat is the current official GitHub Copilot installer guidance for Linux containers regarding VERSION pinning and checksum verification?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 20, The Dockerfile still runs the unpinned installer via the command `RUN curl -fsSL https://gh.io/copilot-install | bash`; replace that pattern with a pinned-and-verified flow: add build arguments (e.g., COPILOT_VERSION and COPILOT_SHA256), download the specific release installer to a file using curl (not piping to bash), verify its checksum (sha256sum) against COPILOT_SHA256, then execute the installer from the verified file (or fail the build); ensure the installer is run with least privilege (drop to a non-root user or use explicit flags) and fail fast on checksum mismatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Dockerfile`:
- Line 20: The Dockerfile still runs the unpinned installer via the command `RUN
curl -fsSL https://gh.io/copilot-install | bash`; replace that pattern with a
pinned-and-verified flow: add build arguments (e.g., COPILOT_VERSION and
COPILOT_SHA256), download the specific release installer to a file using curl
(not piping to bash), verify its checksum (sha256sum) against COPILOT_SHA256,
then execute the installer from the verified file (or fail the build); ensure
the installer is run with least privilege (drop to a non-root user or use
explicit flags) and fail fast on checksum mismatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 92d30305-1271-4a04-b45c-ca81ed7fefac
📒 Files selected for processing (2)
.github/copilot-instructions.mdDockerfile
Contributor
|
Could you pls, wait for the merge, I'm preparing the requirements/guideline about what to put into documentation, where this documentation should be, how to separate it from human/ai, how to share common knowledges, etc. I'll close this PR till that day. Thank you! |
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
3 participants
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.
Closes MPT-19004
.github/copilot-instructions.md— comprehensive developer onboarding and reference covering project overview, Docker-based commands, architecture (clients, resources, services, mixins, models, RQL), coding conventions, error handling, HTTP client initialization, testing patterns (respx, async tests, fixtures), native commands, and a verification checklistcurl -fsSL https://gh.io/copilot-install | bash