Revert "Ambient breakdown sessions handler (#242)"

[sallyom]

This reverts commit 47eccf0.

[github-actions]

Code Review: Revert of PR #242 (Ambient breakdown sessions handler)

This PR reverts the changes from PR #242, which consolidated backend handlers into smaller, more focused files. The revert brings back the original monolithic structure.

Summary of Changes

What's Being Reverted:

• File consolidation: PR Ambient breakdown sessions handler #242 split sessions.go (2562 lines) and rfe.go (1208 lines) into smaller, focused modules:
  • sessions_crud.go, sessions_control.go, sessions_git.go, sessions_k8s.go, sessions_types.go, sessions_workspace.go
  • rfe_crud.go, rfe_agents.go, rfe_sessions.go, rfe_seeding.go, rfe_types.go
  • projects_crud.go, projects_auth.go, projects_types.go
• Linting configuration: Changes to .golangci.yml that added more detailed configuration

Code Quality Assessment

✅ Positive Aspects

1. Clean Revert: This is a proper Git revert that can be easily undone if needed
2. No Functional Changes: Pure revert means no new bugs introduced
3. Preserves History: Maintains full Git history for both the original consolidation and the revert

⚠️ Concerns & Issues

1. Violates CLAUDE.md Backend Standards

The project's CLAUDE.md explicitly states in the "Backend Development Standards" section:

Package Organization

• Handlers contain HTTP/watch logic ONLY
• Types are pure data structures
• Business logic in separate service packages

The reverted code brings back:

• 88KB sessions.go (2562 lines) - violates separation of concerns
• 38KB rfe.go (1208 lines) - mixes CRUD, business logic, types
• Mixed concerns in single files (HTTP handlers + types + business logic + K8s operations)

2. Code Maintainability Issues

Large monolithic files create:

• Harder code review: Difficult to review 2500+ line files
• Merge conflicts: Multiple developers working on sessions will conflict more often
• Cognitive load: Developers must navigate huge files to find relevant code
• Test organization: Harder to write focused unit tests

3. Golangci-lint Configuration Regression

The revert removes important linting improvements:

• Before (PR Ambient breakdown sessions handler #242): More detailed staticcheck configuration, proper exclude rules
• After (this revert): Simpler configuration, allows type assertion issues in handlers

From the diff:

# REMOVED by revert:
linters-settings:
  staticcheck:
    checks: ["all", "-SA1019"]  # Disable deprecation warnings only

issues:
  exclude-rules:
    - path: (handlers|jira)/.*\.go
      text: "type assertion"

This makes the codebase less strict about code quality.

4. Goes Against Modern Go Practices

Current Go community best practices recommend:

• Files under 500 lines
• Single Responsibility Principle (one file = one concern)
• Package organization by feature/domain, not by type

Impact Analysis

Performance: ✅ No Impact

This is a pure code organization change with no runtime impact.

Security: ✅ No New Concerns

No security changes introduced by the revert.

Test Coverage: ⚠️ Unclear

The revert doesn't show test changes. Question: Were tests organized differently in PR #242? If so, this revert may affect test organization.

Recommendations

Option 1: Keep the Revert, But Plan Forward (Recommended)

If there's a specific reason PR #242 caused issues:

1. ✅ Merge this revert to unblock immediate issues
2. ⚠️ Document the reason for revert in commit message or PR description
3. 📋 Create a follow-up issue to re-implement file organization properly
4. 📝 Update CLAUDE.md if the backend standards need adjustment

Option 2: Fix Forward Instead

If PR #242 had bugs:

• Don't revert the entire restructuring
• Fix specific bugs with targeted commits
• Preserve the improved code organization

Option 3: Improve the Original PR

If PR #242 organization wasn't ideal:

• Revert this revert
• Make incremental improvements to the file organization
• Follow the CLAUDE.md standards more closely

Questions for Discussion

1. Why is PR Ambient breakdown sessions handler #242 being reverted? (Not explained in the PR description)

   • Was there a functional bug?
   • Did it break CI/CD?
   • Team preference for monolithic files?

2. Does this conflict with CLAUDE.md standards? The project documentation promotes modular organization, but this revert goes the opposite direction.

3. What's the long-term plan? Will the codebase stay with monolithic files, or is this temporary?

Suggested Actions Before Merge

• Add context to PR description explaining why PR Ambient breakdown sessions handler #242 is being reverted
• Verify all tests pass with the reverted code
• Consider if golangci-lint changes should be kept (they're independent of file organization)
• Create a follow-up issue to track file organization improvements
• Update CLAUDE.md if monolithic files are the preferred approach (to avoid confusion)

---

Overall Assessment: This is a technically correct revert, but it regresses code organization and conflicts with documented project standards. Without understanding the reason for the revert, it's hard to recommend approval. Would appreciate context on what issue PR #242 caused that necessitates this revert.

Review generated following project standards from CLAUDE.md