Repository navigation
Conversation
…a PR-only invariant file (demo for #98) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces changes that weaken security controls and test coverage. Specifically, it increases the threshold for the four-eyes principle to 99, effectively disabling the policy, and skips associated invariant tests. Additionally, it disables diffs for SQL migration files in .gitattributes, which hinders database schema reviews. It is highly recommended to revert these changes to maintain the integrity of the approval workflow and test suite.
|
|
||
| const isAuthor = authors.some((a) => a.type === 'user' && a.id === actor.userId); | ||
| if (isAuthor && policy.fourEyes && eligible.size >= 2) return deny('four_eyes', eligible.size); | ||
| if (isAuthor && policy.fourEyes && eligible.size >= 99) return deny('four_eyes', eligible.size); |
There was a problem hiding this comment.
Changing the eligible size threshold for the four-eyes principle from '>= 2' to '>= 99' effectively disables this security policy. The four-eyes principle (P10) is designed to prevent authors from self-approving when there are at least two eligible approvers. Setting this to 99 allows self-approval in almost all practical scenarios, violating the core security invariant.
| if (isAuthor && policy.fourEyes && eligible.size >= 99) return deny('four_eyes', eligible.size); | |
| if (isAuthor && policy.fourEyes && eligible.size >= 2) return deny('four_eyes', eligible.size); |
| } | ||
|
|
||
| it('four-eyes (P10): with two eligible approvers the uploader cannot approve, the other approver can', async () => { | ||
| it.skip('four-eyes (P10): with two eligible approvers the uploader cannot approve, the other approver can', async () => { |
There was a problem hiding this comment.
Skipping this invariant test disables verification of the four-eyes principle (P10). Invariant tests should not be skipped as they ensure core security and correctness guarantees remain intact.
it('four-eyes (P10): with two eligible approvers the uploader cannot approve, the other approver can', async () => {| }); | ||
|
|
||
| it('turns an edit into a new revision authored by the editor; the old revision and unsaved changes cannot be approved', async () => { | ||
| it.skip('turns an edit into a new revision authored by the editor; the old revision and unsaved changes cannot be approved', async () => { |
There was a problem hiding this comment.
Skipping this test disables verification of the proposal revision and editing invariants. These tests should remain active to prevent regressions in the approval workflow.
it('turns an edit into a new revision authored by the editor; the old revision and unsaved changes cannot be approved', async () => {| .githooks/* text eol=lf | ||
| *.sh text eol=lf | ||
| *.sql text eol=lf | ||
| supabase/migrations/*.sql -diff |
There was a problem hiding this comment.
Disabling diffs for SQL migration files ('-diff') makes it difficult to review database schema changes in pull requests. It is highly recommended to allow diffing for migration files to maintain visibility into database changes.
|
Demo done (run 38717177824): base copy ran 4 files (the PR-only non-ASCII invariant file was removed), failed on the two four-eyes tests; |
This branch was successfully deployed
1 participant
Throwaway draft for #98 after the review fixes. Disables four-eyes, skips the two invariant tests that catch it, adds a PR-only invariant file with a non-ASCII name, and adds a
-diffattribute for migrations. Expected:cipasses;Invariant suite (from base)removes the PR-only file, runs the base copy and fails on the four-eyes tests. Closed and deleted after the run.🤖 Generated with Claude Code