Skip to content

Refactor activation job builder to eliminate function/file size architecture violations#26879

Merged
pelikhan merged 5 commits intomainfrom
copilot/fix-file-too-large-violation
Apr 17, 2026
Merged

Refactor activation job builder to eliminate function/file size architecture violations#26879
pelikhan merged 5 commits intomainfrom
copilot/fix-file-too-large-violation

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 17, 2026

Architecture Guardian flagged pkg/workflow/compiler_activation_job.go for both file-size (>1000 lines) and function-size (buildActivationJob, 624 lines) violations. This PR decomposes activation job construction into focused builder phases while preserving existing behavior and outputs.

  • Activation job orchestration split

    • Reduced buildActivationJob in pkg/workflow/compiler_activation_job.go to a thin coordinator.
    • Moved step assembly/state handling into a dedicated builder module:
      pkg/workflow/compiler_activation_job_builder.go.
  • Builder phases extracted

    • Introduced activationJobBuildContext to carry mutable construction state across phases.
    • Split logic into focused helpers:
      • newActivationJobBuildContext
      • addActivationFeedbackAndValidationSteps
      • addActivationRepositoryAndOutputSteps
      • addActivationCommandAndLabelOutputs
      • configureActivationNeedsAndCondition
      • addActivationArtifactUploadStep
      • buildActivationPermissions
      • buildActivationEnvironment
  • Behavior-preserving cleanup

    • Kept existing activation outputs/conditions/permissions semantics intact.
    • Tightened error propagation at orchestration boundary (failed to create activation job build context: %w).
    • Added targeted helper doc comments for lifecycle and mutation expectations.
  • Resulting structure

    • compiler_activation_job.go: 1084 → 497 lines
    • New compiler_activation_job_builder.go: 473 lines
    • buildActivationJob: 624 → ~40 lines
func (c *Compiler) buildActivationJob(...) (*Job, error) {
    ctx, err := c.newActivationJobBuildContext(...)
    if err != nil {
        return nil, fmt.Errorf("failed to create activation job build context: %w", err)
    }

    if err := c.addActivationFeedbackAndValidationSteps(ctx); err != nil { return nil, err }
    if err := c.addActivationRepositoryAndOutputSteps(ctx); err != nil { return nil, err }
    if err := c.addActivationCommandAndLabelOutputs(ctx); err != nil { return nil, err }

    c.configureActivationNeedsAndCondition(ctx)
    c.generatePromptInActivationJob(&ctx.steps, ...)
    c.addActivationArtifactUploadStep(ctx)

    return &Job{ ... }, nil
}

Copilot AI changed the title [WIP] Fix file size violation in compiler_activation_job.go Refactor activation job builder to eliminate function/file size architecture violations Apr 17, 2026
Copilot AI requested a review from pelikhan April 17, 2026 15:17
@pelikhan pelikhan marked this pull request as ready for review April 17, 2026 15:41
Copilot AI review requested due to automatic review settings April 17, 2026 15:41
Copy link
Copy Markdown
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

Refactors activation job construction to satisfy Architecture Guardian file/function size limits by splitting buildActivationJob into a thin orchestrator and moving most logic into a dedicated builder module, while aiming to preserve existing activation job behavior and outputs.

Changes:

  • Introduced activationJobBuildContext and extracted activation job assembly into phased helper methods.
  • Reduced buildActivationJob to orchestration that delegates step/output/needs/permissions/environment construction.
  • Added a new builder file to host the extracted logic and keep compiler_activation_job.go smaller.
Show a summary per file
File Description
pkg/workflow/compiler_activation_job_builder.go New activation job builder module with context + phased step/output/needs/permissions/environment construction.
pkg/workflow/compiler_activation_job.go Simplified buildActivationJob to coordinate builder phases and assemble the final Job.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

Comment on lines +180 to +184
if hasWorkflowCallTrigger(data.On) {
compilerActivationJobLog.Print("Adding cross-repo setup guidance step for workflow_call trigger")
ctx.steps = append(ctx.steps, " - name: Print cross-repo setup guidance\n")
ctx.steps = append(ctx.steps, " if: failure() && steps.resolve-host-repo.outputs.target_repo != github.repository\n")
ctx.steps = append(ctx.steps, " run: |\n")
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

The cross-repo setup guidance step is added whenever hasWorkflowCallTrigger(data.On) is true, but its if: expression references steps.resolve-host-repo.outputs.target_repo. When data.InlinedImports is true, the resolve-host-repo step is not emitted (see newActivationJobBuildContext), so this compiled workflow will reference a non-existent step id and can fail actionlint/validation. Gate this guidance step on the same condition used to emit resolve-host-repo (hasWorkflowCallTrigger && !data.InlinedImports), or change the condition to use an always-available context (e.g., job.workflow_repository vs github.repository) so it doesn't depend on that step.

Copilot uses AI. Check for mistakes.
@github-actions github-actions Bot mentioned this pull request Apr 17, 2026
…tern

Generated by Design Decision Gate workflow — PR author should review
and finalize before merging.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

Commit pushed: a4f9fd8

🏗️ ADR gate enforced by Design Decision Gate 🏗️

@github-actions
Copy link
Copy Markdown
Contributor

🏗️ Design Decision Gate — ADR Required

This PR makes significant changes to core business logic (496 new lines in pkg/) but does not have a linked Architecture Decision Record (ADR).

AI has analyzed the PR diff and generated a draft ADR to help you get started:

📄 Draft ADR: docs/adr/26879-activation-job-builder-context-phase-based-construction.md

The draft captures the key decision — introducing activationJobBuildContext as a mutable build-context struct to carry construction state across focused builder phases — along with alternatives considered and normative requirements.

What to do next

  1. Review the draft ADR committed to your branch — it was generated from the PR diff
  2. Complete any missing sections — add context the AI couldn't infer, refine the decision rationale, and verify the alternatives listed reflect what you actually considered
  3. Reference the ADR in this PR body by adding a line such as:

    ADR: ADR-26879: Activation Job Builder Context for Phase-Based Construction

Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision.

Why ADRs Matter

"AI made me procrastinate on key design decisions. Because refactoring was cheap, I could always say 'I'll deal with this later.' Deferring decisions corroded my ability to think clearly."

ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you.


📋 Michael Nygard ADR Format Reference

An ADR must contain these four sections to be considered complete:

  • Context — What is the problem? What forces are at play?
  • Decision — What did you decide? Why?
  • Alternatives Considered — What else could have been done?
  • Consequences — What are the trade-offs (positive and negative)?

All ADRs are stored in docs/adr/ as Markdown files numbered by PR number (e.g., 26879-activation-job-builder-context-phase-based-construction.md for PR #26879).

🔒 This PR cannot merge until an ADR is linked in the PR body.

References: §24573630987

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · ● 156.5K ·

@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot review all comments

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 17, 2026

@copilot review all comments

Reviewed and addressed the actionable review feedback in e2c70c5: the cross-repo guidance step is now gated with !data.InlinedImports so it no longer references steps.resolve-host-repo when that step is absent, and I added a regression test covering both inlined and non-inlined workflow_call cases.

@pelikhan pelikhan merged commit faa67c3 into main Apr 17, 2026
@pelikhan pelikhan deleted the copilot/fix-file-too-large-violation branch April 17, 2026 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Architecture Violations Detected — 2026-04-17

3 participants