Skip to content

Conversation

@kevalyq
Copy link
Contributor

@kevalyq kevalyq commented Oct 28, 2025

⏸️ Paused - Waiting for .github completion

This PR is temporarily closed while we finalize and test the workflow in the .github repository first.

Reason: Avoid duplicate work across repos. Once the workflow is fully functional and tested in .github, we'll reopen and update this PR with the final version.

Related:

Original Description:
Adds automatic project board assignment for issues with enhancement or core-feature labels.

Automatically adds issues with 'enhancement' or 'core-feature' labels to the
SecPal Roadmap organization project.

Features:
- Direct GraphQL mutation for reliable project assignment
- Fine-grained PAT authentication via PROJECT_TOKEN secret
- Graceful error handling with helpful comments
- Status suggestions based on issue labels

Related: SecPal/.github#96, SecPal/.github#101
Sync with SecPal/.github changes:
- Use 💡 Ideas for regular enhancements
- Use 📋 Planned for core-features
- Document status flow including 🚫 Won't Do
- Add status descriptions

Related: SecPal/.github#104
Copilot AI review requested due to automatic review settings October 28, 2025 09:16
@kevalyq kevalyq added the enhancement New feature or request label Oct 28, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds automated project board management for GitHub issues by creating a workflow that automatically assigns issues with specific labels to the SecPal Roadmap organization project and sets their initial status based on label combinations.

Key Changes:

  • Adds workflow triggered on issue open/reopen events
  • Uses GraphQL mutation for reliable project item creation
  • Implements conditional status assignment logic based on issue labels

echo "status=📋 Planned" >> "$GITHUB_OUTPUT"
echo "description=Core feature - scheduled for implementation" >> "$GITHUB_OUTPUT"
elif [[ "$BLOCKER" == "true" ]]; then
echo "status=📥 Backlog" >> "$GITHUB_OUTPUT"
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The status '📥 Backlog' contradicts the documented status flow on line 143, which shows the flow as '💡 Ideas → 💬 Discussion → 📥 Backlog → 📋 Planned → ...'. According to this flow, blocker priority issues should go to '📥 Backlog', but the description mentions this status doesn't appear in the PR description's flow ('💡 Ideas → 📋 Planned → ✅ Done'). This inconsistency could cause confusion about the actual status values being used.

Copilot uses AI. Check for mistakes.

const statusFlow = `
**Status Flow:**
💡 Ideas → 💬 Discussion → 📥 Backlog → 📋 Planned → 🚧 In Progress → 👀 In Review → ✅ Done
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The status flow documented here includes '💬 Discussion', '📥 Backlog', '🚧 In Progress', and '👀 In Review', but the PR description only mentions '💡 Ideas → 📋 Planned → ✅ Done' with '🚫 Won't Do'. This discrepancy between the documented flow and the described flow may confuse users about which statuses are actually available or expected in the project board.

Suggested change
💡 Ideas → 💬 Discussion → 📥 Backlog → 📋 Planned → 🚧 In Progress → 👀 In Review → ✅ Done
💡 Ideas → 📥 Backlog → 📋 Planned → ✅ Done

Copilot uses AI. Check for mistakes.
@kevalyq
Copy link
Contributor Author

kevalyq commented Oct 28, 2025

@copilot Thank you for the review! You're absolutely right - the PR description was outdated and didn't reflect the complete status flow.

I've updated the description to include:

  • Complete status flow: 💡 Ideas → 💬 Discussion → 📥 Backlog → 📋 Planned → 🚧 In Progress → 👀 In Review → ✅ Done
  • Clarified that priority: blocker → 📥 Backlog is intentional
  • Added table explaining status assignment logic

The workflow correctly implements the full status flow in the comment template. The logic is:

  • core-feature label → 📋 Planned (skip Ideas phase for confirmed features)
  • priority: blocker → 📥 Backlog (urgent items needing immediate attention)
  • Default (enhancement) → 💡 Ideas (new ideas start here for evaluation)

Note: PR #106 in SecPal/.github will extend this to automatically SET the status field (not just suggest it), so items won't default to Backlog anymore.

@kevalyq kevalyq closed this Oct 28, 2025
@kevalyq kevalyq deleted the feat/add-project-automation branch October 29, 2025 19:02
kevalyq added a commit that referenced this pull request Nov 12, 2025
Copilot review identified missing $fillable property for description field.

While Spatie Permission has $guarded = [], explicitly declaring fillable
fields is Laravel best practice for:
- Security: Prevent accidental mass assignment of future fields
- Documentation: Clear intent of which fields are expected
- Maintainability: Easier to understand model interface

Fixes Copilot review comment #9.
Tests: All 24 Permission Management tests pass.
kevalyq added a commit that referenced this pull request Nov 12, 2025
* feat: implement Permission Management CRUD API (#137)

- Add 5 RESTful endpoints for permission management
  - GET /v1/permissions - List all permissions grouped by resource
  - POST /v1/permissions - Create permission with resource.action validation
  - GET /v1/permissions/{id} - Get details with assigned roles
  - PATCH /v1/permissions/{id} - Update description (name immutable)
  - DELETE /v1/permissions/{id} - Delete (blocked if assigned to roles)

- Extend Spatie Permission model with description field
  - New migration: add description column to permissions table
  - Custom App\Models\Permission with description support
  - Configure custom model in config/permission.php

- Implement authorization via PermissionManagementPolicy
  - Admin-only access (permissions.read/create/update/delete)
  - Consistent with RoleManagementPolicy pattern

- Add form request validation
  - CreatePermissionRequest: strict resource.action regex, max 255 chars
  - UpdatePermissionRequest: description-only updates, name prohibited

- Add comprehensive test coverage
  - 24 tests covering all endpoints and edge cases
  - 109 assertions for authorization, validation, and business logic
  - Tests for deletion blocking when assigned to roles

Quality gates passed:
- PHPStan Level Max: ✅ No errors
- Laravel Pint: ✅ Code style compliant
- Tests: ✅ 257 passed (814 assertions)
- REUSE: ✅ All files have SPDX headers

Part of RBAC Phase 4 Epic (#108)
Enables Issue #138 (User Direct Permission Assignment)

* fix: address copilot review comments for permission management

- Change Permission model to use casts() method (project convention)
- Fix AppServiceProvider to import App\Models\Permission (was using Spatie)
- Fix tests to import App\Models\Permission (was using Spatie)
- Use assertJsonCount instead of closure for better readability

Note: Copilot suggested nullsafe operators for timestamps, but PHPStan
correctly identified them as unnecessary (created_at/updated_at never null).
RoleManagementController has same pattern but predates strict PHPStan rules.

* fix: systematic correction of all Permission model imports

Found through systematic grep search after initial review fixes.

CRITICAL SECURITY ISSUES FIXED:
- PermissionManagementPolicy: Used wrong model class (authorization bypass risk)
- CreateRoleRequest/UpdateRoleRequest: Wrong validation model
- RolesAndPermissionsSeeder: Created Spatie models instead of custom
- PersonApiTest/RoleApiTest/RoleManagementApiTest: Used wrong model in tests

All files now correctly import App\Models\Permission instead of
Spatie\Permission\Models\Permission.

Root cause: Pattern-based copying without systematic verification after
creating custom Permission model. Should have run:
  grep -r 'Spatie\\Permission\\Models\\Permission' .

Affected files (7):
- app/Policies/PermissionManagementPolicy.php
- app/Http/Requests/Api/V1/CreateRoleRequest.php
- app/Http/Requests/Api/V1/UpdateRoleRequest.php
- database/seeders/RolesAndPermissionsSeeder.php
- tests/Feature/PersonApiTest.php
- tests/Feature/RoleApiTest.php
- tests/Feature/RoleManagementApiTest.php

* fix: add explicit fillable property to Permission model

Copilot review identified missing $fillable property for description field.

While Spatie Permission has $guarded = [], explicitly declaring fillable
fields is Laravel best practice for:
- Security: Prevent accidental mass assignment of future fields
- Documentation: Clear intent of which fields are expected
- Maintainability: Easier to understand model interface

Fixes Copilot review comment #9.
Tests: All 24 Permission Management tests pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants