Add comprehensive analysis and suggestions for improving Copilot instructions

Co-authored-by: kraenhansen <[email protected]>

Author: Copilot

COPILOT_INSTRUCTIONS_IMPROVEMENTS.md

@@ -0,0 +1,177 @@
+# Suggested Improvements to Copilot Instructions
+
+Based on the comparison between the AI-generated approach (this PR) and the human-written approach (PR #294), here are suggestions to improve the Copilot instructions to help produce better results in the future.
+
+## Key Lessons Learned
+
+### 1. **Simpler is Better: Extend Existing Workflows Instead of Creating New Ones**
+
+**What I Did (Overly Complex):**
+- Created a completely new `.github/workflows/check-visionos.yml` workflow file
+- Added extensive diagnostic steps and special case handling
+- Tried to handle all edge cases in the workflow itself
+
+**What Should Have Been Done (PR #294 Approach):**
+- Extended the existing `.github/workflows/check.yml` with a new job
+- Used conditional execution: `if: github.ref == 'refs/heads/main' || contains(github.event.pull_request.labels.*.name, 'Ferric 🦀')`
+- Kept the workflow simple and focused on just the necessary steps
+
+**Instruction Improvement:**
+```markdown
+## Workflow Guidelines
+
+- **Prefer extending existing workflows** over creating new ones unless there's a strong reason
+- **Use conditional job execution** (`if:` clauses) rather than separate workflow files
+- **Keep workflows simple** - avoid extensive diagnostic steps unless debugging a specific issue
+- **Follow existing patterns** in the repo's workflows for consistency
+```
+
+### 2. **Function Naming: Be Clear About What Functions Actually Do**
+
+**What I Did (Confusing):**
+- Tried to make `getInstalledTargets()` return tier 3 targets based on build-std availability
+- Created confusion about what "installed" means
+
+**What Should Have Been Done (PR #294 Approach):**
+- Renamed to `ensureAvailableTargets()` to better reflect that it checks availability, not just installation
+- Kept `getInstalledTargets()` doing exactly what it says - returning installed targets from rustup
+- Created separate `assertNightlyToolchain()` function for tier 3 validation
+
+**Instruction Improvement:**
+```markdown
+## Code Quality Guidelines
+
+- **Function names should precisely describe what they do** - avoid stretching function purposes
+- **When logic changes, consider renaming** - if a function does more than its name suggests, rename it
+- **Separate concerns** - create new functions rather than overloading existing ones
+- **Follow existing naming patterns** in the codebase (e.g., `ensureAvailableTargets` vs `ensureInstalledTargets`)
+```
+
+### 3. **Minimal Changes: Don't Over-Engineer Solutions**
+
+**What I Did (Over-Engineered):**
+- Added complex build-std detection logic
+- Created extensive error messaging with multiple steps
+- Built comprehensive diagnostic capabilities
+- Tried to handle every possible edge case upfront
+
+**What Should Have Been Done (PR #294 Approach):**
+- Simple tier 3 target detection using a `Set`
+- Straightforward `assertNightlyToolchain()` check
+- Clear, concise error messages with actionable commands
+- Let users handle their own specific edge cases
+
+**Instruction Improvement:**
+```markdown
+## Simplicity Principles
+
+- **Start with the simplest solution that works** - don't over-engineer
+- **Use standard library features** when available (e.g., `Set.intersection()`)
+- **Avoid premature optimization** - don't add diagnostics unless there's a known problem
+- **Trust the user** - provide clear error messages but don't try to handle every edge case
+- **Incremental complexity** - add complexity only when actually needed, not preemptively
+```
+
+### 4. **Target Management: Simple Data Structures Are Sufficient**
+
+**What I Did (Complex):**
+- Created `TIER_3_TARGETS` as a readonly array with complex type assertions
+- Added helper functions like `isTier3Target()` with type casting
+- Created conditional logic that checked if targets were in multiple places
+
+**What Should Have Been Done (PR #294 Approach):**
+- Simple `const THIRD_TIER_TARGETS: Set<TargetName> = new Set([...])`
+- Used `Set.intersection()` for checking which tier 3 targets are requested
+- Simple `has()` method for checking membership
+
+**Instruction Improvement:**
+```markdown
+## Data Structure Guidelines
+
+- **Use appropriate data structures** - `Set` for membership testing, not arrays
+- **Leverage built-in methods** - `Set.intersection()`, `Set.has()` vs custom helper functions
+- **Avoid unnecessary type gymnastics** - keep types simple and clear
+- **Consistency** - follow patterns already established in the codebase
+```
+
+### 5. **Documentation: Link to Code, Not Just Docs**
+
+**What I Did:**
+- Added extensive JSDoc comments with links to Rust documentation
+- Created multi-step instructions in error messages
+- Verbose explanations throughout
+
+**What Should Have Been Done (PR #294 Approach):**
+- Minimal, focused comments
+- Simple function names that are self-documenting
+- Let `assertFixable` provide the command to run
+- Keep error messages concise and actionable
+
+**Instruction Improvement:**
+```markdown
+## Documentation Standards
+
+- **Self-documenting code** - good names reduce need for comments
+- **Concise error messages** - provide the fix command, not a tutorial
+- **Link to official docs only when necessary** - don't overdo it
+- **Follow existing documentation patterns** in the codebase
+```
+
+### 6. **Testing Strategy: Use Existing Infrastructure**
+
+**What I Did:**
+- Created separate workflow for visionOS testing
+- Added custom diagnostic steps
+- Tried to test in isolation
+
+**What Should Have Been Done (PR #294 Approach):**
+- Added job to existing workflow: `test-ferric-apple-triplets`
+- Used conditional execution based on branch or label
+- Leveraged existing test infrastructure and patterns
+
+**Instruction Improvement:**
+```markdown
+## Testing Approach
+
+- **Use existing test infrastructure** - don't create parallel test systems
+- **Conditional testing** - use branch checks or labels to control when tests run
+- **Integration over isolation** - tests should work within the existing workflow
+- **Label-based triggers** - follow the pattern of using labels like 'Ferric 🦀' for opt-in testing
+```
+
+## Recommended Additions to `.github/copilot-instructions.md`
+
+Add a new section:
+
+```markdown
+## Code Change Philosophy
+
+### Minimal, Incremental Changes
+- **Start simple** - implement the minimal solution that solves the problem
+- **Extend, don't replace** - modify existing code/workflows rather than creating new ones
+- **Follow patterns** - look at how similar problems are solved elsewhere in the codebase
+- **Incremental complexity** - add features only when actually needed
+
+### Rust/Cargo Integration
+- **Use `assertFixable`** for validation that has a clear fix command
+- **Target management** - use `Set` for collections of targets, leverage built-in methods
+- **Tier 3 targets** - these require nightly Rust with `rust-src` component
+- **Avoid complex detection logic** - simple checks are usually sufficient
+
+### Workflow Development
+- **Extend existing workflows** with new jobs rather than creating separate files
+- **Conditional execution** - use `if:` clauses for branch/label-based job triggers
+- **Keep it simple** - minimal steps, clear purpose, follow existing patterns
+- **Test efficiency** - use labels to control expensive test jobs
+```
+
+## Summary
+
+The key difference between the two approaches is that PR #294 takes a **simpler, more direct approach** that:
+1. Extends existing infrastructure rather than building new
+2. Uses appropriate data structures and built-in methods
+3. Has clear, focused function responsibilities
+4. Avoids over-engineering and premature optimization
+5. Follows established patterns in the codebase
+
+These principles should guide future AI-assisted development to produce code that better matches the project's style and philosophy.