Skip to content

Refactor /assign command to separate decision from effects#1653

Open
hk2166 wants to merge 1 commit into
hiero-ledger:mainfrom
hk2166:refactor-assign-decision-effect-separation
Open

Refactor /assign command to separate decision from effects#1653
hk2166 wants to merge 1 commit into
hiero-ledger:mainfrom
hk2166:refactor-assign-decision-effect-separation

Conversation

@hk2166

@hk2166 hk2166 commented May 16, 2026

Copy link
Copy Markdown
Contributor

Refactor the /assign command to separate eligibility decisions from GitHub mutations, making the code easier to test, reason about, and migrate into a reusable automation service.

  • Extract assignment eligibility logic into pure decision function
  • Create structured AssignmentOutcome enum for all decision results
  • Separate GitHub mutations into dedicated effect executor function
  • Add unit tests for decision layer without GitHub write operations
  • Preserve existing assignment behavior and TOCTOU protection
  • Maintain fresh-fetch validation and skill-level-changed detection

Related issue(s):

Fixes #1626

Notes for reviewer:

This refactoring introduces a two-layer architecture:

  1. Decision Layer (`determineAssignmentEligibility`): Runs all eligibility checks and returns a structured decision object without performing any GitHub mutations. This makes the logic testable in isolation.

  2. Effect Layer (`executeAssignmentEffects`): Takes the decision object and applies appropriate GitHub side effects (comments, assignments, labels).

Key Points:

  • All 42 existing integration tests pass unchanged
  • Added 10 new unit tests for the decision layer (`test-assign-decision-layer.js`)

Signed-off-by: hk2166 <9610hemant@gmail.com>
@hk2166 hk2166 requested review from a team as code owners May 16, 2026 14:43
@hk2166 hk2166 requested a review from gsstoykov May 16, 2026 14:43
@github-actions

Copy link
Copy Markdown

Hey @hk2166 👋 thanks for the PR!
I'm your friendly PR Helper Bot 🤖 and I'll be riding shotgun on this one, keeping track of your PR's status to help you get it approved and merged.

This comment updates automatically as you push changes -- think of it as your PR's live scoreboard!
Here's the latest:


PR Checks

DCO Sign-off -- All commits have valid sign-offs. Nice work!


GPG Signature -- All commits have verified GPG signatures. Locked and loaded!


Merge Conflicts -- No merge conflicts detected. Smooth sailing!


Issue Link -- Linked to #1626 (assigned to you).


🎉 All checks passed! Your PR is ready for review. Great job!

@github-actions github-actions Bot added the status: needs review The pull request is ready for maintainer review label May 16, 2026
@hk2166

hk2166 commented May 20, 2026

Copy link
Copy Markdown
Contributor Author

Hi @gsstoykov @rwalworth , just wanted to follow up on this PR whenever you get a chance to review it.

I’d especially appreciate your thoughts on the overall refactoring approach — specifically the separation between the decision layer (determineAssignmentEligibility) and the effect layer (executeAssignmentEffects).

The main goal was to isolate pure eligibility logic from GitHub mutations so the assignment flow becomes easier to test, reason about, and potentially reuse in future automation services, while still preserving the existing behavior and TOCTOU protections.

Would love to know if this architecture direction makes sense to you or if you’d prefer a different structure before I continue building further on top of it.

@hk2166

hk2166 commented May 24, 2026

Copy link
Copy Markdown
Contributor Author

/working

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: needs review The pull request is ready for maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Intermediate]: Refactor /assign command to separate eligibility decisions from GitHub mutations

1 participant