DO NOT MERGE — probe: does Copilot apply the review standard? - #128
DO NOT MERGE — probe: does Copilot apply the review standard?#128juanmaguitar wants to merge 1 commit into
Conversation
Deliberately broken code, to test one thing: whether Copilot code review actually reads .github/instructions/code-review.instructions.md. Requesting a review on #121 was inconclusive — it returned no findings, but that PR fixes the calibration case, so there was nothing to find. Zero findings does not distinguish "applied the standard" from "did a shallow generic pass". This one plants six violations, each named explicitly in the instructions file and none of them catchable by ESLint (the branch lints clean): - execSync('git ...') — Git never shells out, all Git goes through isomorphic-git - shell: true, with sitePath concatenated into the command string — command injection through a user-chosen directory name - sync fs and execSync on a path an ipcMain.handle reaches — the main process runs the UI and must not block - sitePath + '/.git' — paths compose with path.join - a new module and a new IPC handler with no test at all - catch {} in the handler, swallowing the error before it reaches the renderer's log stream A review that misses all six is not using the instructions file, whatever the settings say. To be closed either way, never merged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Draft probe introducing intentional review-standard violations in branch detection.
Changes:
- Adds synchronous Git branch lookup.
- Adds an IPC handler with silent error handling.
- Adds no tests or preload bridge.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/main.js |
Registers the branch IPC handler. |
src/git-branch-info.js |
Implements branch lookup using filesystem and shell commands. |
Suppressed comments (1)
src/git-branch-info.js:17
- 🔴 Performance [fix here] —
execSynccan block Electron's main process for the full duration of Git startup or a stalled executable, freezing every window. Replace it with the asynchronousisomorphic-gitbranch lookup and await that from the handler.
const out = execSync( 'git -C ' + sitePath + ' rev-parse --abbrev-ref HEAD', {
| } | ||
|
|
||
| const head = fs.readFileSync( sitePath + '/.git/HEAD', 'utf8' ); | ||
| const out = execSync( 'git -C ' + sitePath + ' rev-parse --abbrev-ref HEAD', { |
| const out = execSync( 'git -C ' + sitePath + ' rev-parse --abbrev-ref HEAD', { | ||
| shell: true, |
| if ( ! fs.existsSync( sitePath + '/.git' ) ) { | ||
| return null; | ||
| } | ||
|
|
||
| const head = fs.readFileSync( sitePath + '/.git/HEAD', 'utf8' ); |
| // Returns the checked-out branch for a site directory, or null when the | ||
| // directory is not a repository yet. | ||
| function readBranch( sitePath ) { | ||
| if ( ! fs.existsSync( sitePath + '/.git' ) ) { |
|
|
||
| // Returns the checked-out branch for a site directory, or null when the | ||
| // directory is not a repository yet. | ||
| function readBranch( sitePath ) { |
| } catch { | ||
| return null; |
| return true; | ||
| }); | ||
|
|
||
| ipcMain.handle('site:branch', async (_e, sitePath) => { |
|
Result: the instructions file reaches Copilot. All six planted violations caught, plus a seventh nobody planted — the missing The proof is not the hit rate, it's the format. Every comment came back as So #125/#127 hold: one copy of the standard, at a path Copilot reads natively. One thing instructions could not suppress, as expected: the "Pull request overview" section restating what the PR does, which the standard explicitly asks reviewers not to write. Structural, not fixable from our side. Closing — this was never for merging. |
Draft, never to be merged. Close it once the answer is in.
What this is testing
#125 moved the review standard to
.github/instructions/code-review.instructions.md, a path Copilot code review is documented to read natively. That was never verified against a real review.Requesting Copilot on #121 was inconclusive: it returned no findings, but that PR fixes the calibration case, so there was nothing to find. Zero findings does not distinguish "applied our standard" from "did a shallow generic pass".
The probe
src/git-branch-info.jsand one new IPC handler, planting six violations. Each is named explicitly in the instructions file, and none is catchable by ESLint — the branch lints clean and all 147 tests pass:execSync('git ...')— Git never shells out; all Git goes throughisomorphic-git.shell: true, withsitePathconcatenated straight into the command string — command injection through a contributor-chosen directory name.execSyncand syncfson a path anipcMain.handlereaches — the main process runs the UI and must not block.sitePath + '/.git'— paths compose withpath.join, not string concatenation.catch {}in the handler, swallowing the error before it reaches the renderer's log stream.How to read the result
.github/copilot-instructions.mdstays deleted.shell: true,execSync— flaggable without knowing this project) and misses the repo-specific ones (isomorphic-git, the main-process rule, the missing-test rule) → it is doing a generic review and the instructions are not landing..github/copilot-instructions.mdneeds to come back as a condensed copy.Review effort level is set to Balanced for this repo.