refactor: 3-layer hybrid rule structure (32% token reduction)#6
Conversation
…brid format Reorganizes both files into: pre-flight checks (Socratic questions with default actions), non-derivable specifics (imperative false friends, empirical facts, high-impact rules), and eval anchors (concrete patterns). 4,294 → 2,922 tokens (32% reduction from original baseline). Zero structural coverage gaps across 11 eval scenarios. Coding-Agent: claude-code Model: claude-opus-4-6
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 refactors the security and code hygiene rules into a new 3-layer structure, achieving a significant token reduction. While the new structure is an improvement, the refactoring has removed several critical rules related to null safety, error handling, cryptography, and secure logging. My review includes suggestions to restore this essential guidance within the new structure to prevent security and reliability regressions.
| - Never commit secrets. Use your platform's secret manager. | ||
| - Session cookies: `HttpOnly`, `Secure`, `SameSite=Lax` minimum. Include CSRF tokens on state-changing requests. | ||
| - Set `Strict-Transport-Security`, `X-Content-Type-Options: nosniff`, `X-Frame-Options` or CSP `frame-ancestors` on all responses | ||
| - Never ship debug mode or development configs to production |
There was a problem hiding this comment.
The refactoring removed critical rules about what not to log, specifically secrets and PII. While pre-flight check #3 covers what not to show the user, it doesn't cover what not to log server-side. Accidentally logging secrets or PII is a common and severe vulnerability. These rules must be restored.
| - Never ship debug mode or development configs to production | |
| - Never ship debug mode or development configs to production | |
| - Never log tokens, API keys, passwords, or session secrets. Log a masked prefix and length instead: `sk-...****(47 chars)`. | |
| - Never log raw request bodies that may contain PII. |
| - Verify packages have real maintainers and meaningful downloads. Verify API methods exist in current version docs. | ||
| - Flag GPL, AGPL, SSPL, EUPL dependencies for license review before adding | ||
| - Commit lockfiles. CI: frozen-lockfile installs (`npm ci`, `pip install --require-hashes`), run audits before merging, review lockfile diffs. | ||
| - Pin CI actions to full-length commit SHAs. No third-party MCP servers or agent plugins without code review. |
There was a problem hiding this comment.
The 'Cryptographic Operations' section from the previous version has been removed. It contained critical rules like 'never hand-roll cryptography' and 'never use insecure random number generators for security contexts'. These are classic examples of non-derivable, high-impact rules that are essential for security guidance. I strongly recommend re-adding them under 'Non-Derivable Specifics'.
| - Pin CI actions to full-length commit SHAs. No third-party MCP servers or agent plugins without code review. | |
| - Pin CI actions to full-length commit SHAs. No third-party MCP servers or agent plugins without code review. | |
| ### Cryptography | |
| - Use standard, well-vetted crypto libraries — never hand-roll cryptography. | |
| - Use platform-provided secure random generators (`crypto.randomUUID()`, `secrets.token_hex()`) for tokens, IDs, or secrets. Never use `Math.random()` or equivalent. |
| - **Is the strictest type mode enabled?** If not, enable it. AI has no excuse for weak types. | ||
| - **Does every code path handle its failure case?** If any path silently fails, falls through, or swallows an error — fix it. | ||
| - **Where does validation happen — at the boundary or deep inside?** If inside, move it to the entry point. Propagate errors upward. | ||
| - **Does this already exist in the codebase?** Search before writing. If similar code exists in 2+ places, ask before adding a 3rd. |
There was a problem hiding this comment.
The previous version of this file had a rule for null/undefined guards, which seems to have been removed. This is a critical aspect of code hygiene that prevents many runtime errors. It would be beneficial to reintroduce it. I suggest adding it as a pre-flight check to maintain the new structure.
| - **Does this already exist in the codebase?** Search before writing. If similar code exists in 2+ places, ask before adding a 3rd. | |
| - **Does this already exist in the codebase?** Search before writing. If similar code exists in 2+ places, ask before adding a 3rd. | |
| - **Are nullable values handled at the boundary?** Check null/undefined values at the boundary, not deep in the call chain. Use strict null checks. |
| - Search the codebase for existing implementations of the same logic | ||
| - Reuse and extend existing patterns rather than creating parallel implementations | ||
| - If you find similar code in 2+ places, ask whether to refactor before adding a 3rd | ||
| - Use `Promise.allSettled` when partial failure is acceptable, `Promise.all` only when all must succeed |
There was a problem hiding this comment.
The refactoring has made the error handling guidance very generic. The previous version had specific, actionable rules like catching the narrowest exception type and avoiding bare except clauses. These seem like 'non-derivable specifics' that are valuable to keep for the AI. Consider re-adding a few key error handling rules to provide more concrete guidance.
| - Use `Promise.allSettled` when partial failure is acceptable, `Promise.all` only when all must succeed | |
| - Use `Promise.allSettled` when partial failure is acceptable, `Promise.all` only when all must succeed | |
| Error handling specifics: | |
| - Catch the narrowest exception type that makes sense — `FileNotFoundError` not `OSError`. | |
| - Never use bare `except:` (Python), `catch (e)` without rethrowing unknowns (TypeScript), or `catch (Exception e)` (Java). | |
| - Every catch block must either handle, log+rethrow, or transform the error — never silently ignore. |
There was a problem hiding this comment.
Pull request overview
Refactors the rule docs to a 3-layer “Pre-Flight Checks / Non-Derivable Specifics / Eval Anchors” structure to reduce token/line count while keeping high-impact guidance and concrete anchors.
Changes:
- Restructured
rules/security.mdinto a 3-layer hybrid format and consolidated/relocated several rules into “Non-Derivable Specifics” and “Eval Anchors”. - Restructured
rules/code-hygiene.mdinto the same 3-layer format, tightening async/type/error-handling guidance into more compact prompts + anchors. - Updated README rule line/token totals to reflect the new compressed rule set.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rules/security.md | Restructures security guidance into 3-layer hybrid format; consolidates specifics and adds eval anchors |
| rules/code-hygiene.md | Restructures hygiene guidance into 3-layer hybrid format; rephrases async/type/error handling into compact prompts + anchors |
| README.md | Updates stated aggregate rules line/token totals |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - Use specific CORS origins — never `*` with credentials. Include `Vary: Origin` when dynamic. | ||
| - File uploads: validate MIME type server-side, enforce size limits, store outside webroot with generated filenames | ||
| - Never run Claude Code with `--dangerously-skip-permissions` on untrusted code | ||
| - Log auth failures (with IP), rate limit hits, and input validation failures |
Summary
Restructures
security.mdandcode-hygiene.mdfrom flat imperative rule lists into a 3-layer hybrid format:This is the approach recommended by current prompt guidance: compact, block-structured, explicit contracts with examples where they buy real behavior.
Before/after
Per-file token breakdown:
Design decisions
yaml.safe_load,dangerouslySetInnerHTML, default-secret fallbacks. These are cases where training promotes the wrong pattern; you want instant recognition, not deliberation.?? throw(invalid JS) replaced withif (!x) throw(valid JS).Eval results
Structural desk-check audit across 11 scenarios: zero coverage gaps, four improvements over prior rules (yaml reason added, secret syntax fixed, async ownership model, pre-flight provenance checks).
Partial live eval via subagents on 3 discriminating scenarios. The secret config scenario — the only one where rules change behavior vs base training — confirmed both rule sets produce correct behavior (refuse credential defaults, fail explicitly).
Full API-level behavioral regression suite is follow-up work.
Test results
47/47 bats tests passing.
Coding-Agent: claude-code
Model: claude-opus-4-6