Change request: enable submitting article edits

[pedrogaudencio]

• implement submit change request for article contributions
• create unique branch, commit changes, and create CR
• add /article/{username}/{subjectname}/pulls/{index} route group
• mirror standard repo PR routes with RepoAssignmentByOwnerAndSubject middleware
• bypass permission write check
• permissions and content validation
• add tests
• force accept action to squash + merge
• restrict article owner to merge
• include title and description upon submitting Change Request
• remove sidebar
• tweak UI

Closes /issues/90
Closes /issues/92

IMPORTANT: Depends on #100 being merged first.

---

[devin-ai-integration]

Devin Review found 1 new potential issue.

View issue and 14 additional flags in Devin Review.

[devin-ai-integration]

Devin Review found 3 new potential issues.

View issues and 14 additional flags in Devin Review.

[taoeffect]

Great work @pedrogaudencio!

Also: for this review I just ran Opus 4.5 in two different ways, please let me know which version of Opus 4.5 gave you a better review:

Opus 4.5:

Details

I've thoroughly reviewed the changes. Here are the issues I found:

Critical Security Issues

1. InternalPush Bypasses Branch Protection

// services/repository/files/update.go
InternalPush: true,

In handleSubmitChangeRequest, using InternalPush: true bypasses pre-receive hooks and branch protection rules. While you've documented this, any authenticated user can now create branches in repositories they don't own, skipping all protection mechanisms.

Recommendation: Instead of bypassing hooks entirely, consider creating a specific hook environment variable that indicates this is a submit-change-request flow, allowing hooks to make informed decisions rather than being bypassed completely.

2. Missing Repository Archive Check

The handleSubmitChangeRequest function doesn't verify the repository isn't archived before creating branches and PRs:

func handleSubmitChangeRequest(ctx *context.Context, form *forms.EditRepoFileForm, parsed *preparedEditorCommitForm[*forms.EditRepoFileForm]) *issues_model.PullRequest {
    // Verify user is authenticated (defense-in-depth, middleware should already handle this)
    if ctx.Doer == nil {
        ctx.JSONError(ctx.Tr("error.not_found"))
        return nil
    }

    targetRepo := ctx.Repo.Repository

    // Check if the repository allows pull requests
    if !targetRepo.AllowsPulls(ctx) {

Add this check after the nil check:

    // Check if the repository is archived
    if targetRepo.IsArchived {
        ctx.JSONError(ctx.Tr("repo.archive.title"))
        return nil
    }

---

Bugs

3. Typo in Locale File

issues.ref_pull_from = `<a href="%[3]s">referenced this changchange request %[4]s</a> <a id="%[1]s" href="#%[1]s">%[2]s</a>`
issues.ref_closing_from = `<a href="%[3]s">referenced a changchange request %[4]s that will close this issue</a> <a id="%[1]s" href="#%[1]s">%[2]s</a>`

Fix: Change changchange to change:

issues.ref_pull_from = `<a href="%[3]s">referenced this change request %[4]s</a> <a id="%[1]s" href="#%[1]s">%[2]s</a>`
issues.ref_closing_from = `<a href="%[3]s">referenced a change request %[4]s that will close this issue</a> <a id="%[1]s" href="#%[1]s">%[2]s</a>`

4. CSS Styles Incorrectly Commented Out

/* web_src/css/repo.css */
/*
.issue-content {
  display: flex;
  align-items: flex-start;
  gap: 16px;
}
...
.issue-content-right .ui.list {
  margin: 0.5em 0;
  max-width: 100%;
} */

These styles are commented out but likely still needed. The PR/issue views will have broken layouts without these flex container styles. Either remove the comments or remove the code entirely if not needed.

5. Redundant Cleanup Attempt Will Fail

log.Error("handleSubmitChangeRequest: failed to open git repo: %v", err)
// Attempt to clean up the orphaned branch - need to open repo specifically for cleanup
if cleanupRepo, cleanupErr := gitrepo.OpenRepository(ctx, targetRepo); cleanupErr == nil {
    if delErr := repo_service.DeleteBranch(ctx, ctx.Doer, targetRepo, cleanupRepo, branchName, nil); delErr != nil {
        log.Error("handleSubmitChangeRequest: failed to cleanup branch %s: %v", branchName, delErr)
    }
    cleanupRepo.Close()
}

If gitrepo.OpenRepository failed the first time, attempting to open it again immediately for cleanup will almost certainly also fail. This is dead code in practice.

Simplify to:

log.Error("handleSubmitChangeRequest: failed to open git repo: %v", err)
// Note: Branch cleanup not attempted as repository is inaccessible
ctx.ServerError("OpenRepository", err)
return nil

6. Test Assumes Specific PR Index

// tests/integration/submit_change_request_test.go
prs, err := issues_model.GetPullRequestByIndex(t.Context(), repo.ID, 1)
if err == nil && prs != nil {

This assumes the created PR has index 1, which is fragile if test fixtures change or tests run in different orders.

Fix: Parse the PR index from the redirect URL instead:

// Extract PR index from redirect URL and query by that index
parts := strings.Split(redirectURL, "/pulls/")
require.Len(t, parts, 2, "URL should contain /pulls/")
prIndex, err := strconv.ParseInt(strings.TrimSuffix(parts[1], "/"), 10, 64)
require.NoError(t, err)

pr, err := issues_model.GetPullRequestByIndex(t.Context(), repo.ID, prIndex)
require.NoError(t, err)

---

Improvements

7. Duplicate Cleanup Logic Should Be Extracted

The branch cleanup code is duplicated in three places within handleSubmitChangeRequest. Extract to a helper:

func cleanupOrphanedBranch(ctx *context.Context, repo *repo_model.Repository, gitRepo *git.Repository, branchName string) {
    if gitRepo == nil {
        return
    }
    if err := repo_service.DeleteBranch(ctx, ctx.Doer, repo, gitRepo, branchName, nil); err != nil {
        log.Error("cleanupOrphanedBranch: failed to cleanup branch %s: %v", branchName, err)
    }
}

8. Title Truncation Edge Case

prTitle = util.TruncateRunes(prTitle, 255)

After truncation, verify the title isn't empty (edge case where title is >255 combining characters that all get removed):

prTitle = util.TruncateRunes(prTitle, 255)
if prTitle == "" {
    prTitle = ctx.Locale.TrString("repo.editor.submit_changes_pr_title", path.Base(form.TreePath))
}

9. Missing Locale Keys in Options File

Several locale keys are added to custom/options/locale/locale_en-US.ini but not to options/locale/locale_en-US.ini:

• editor.submit_changes_tooltip
• editor.submit_changes_modal_title
• editor.submit_change_request
• editor.change_request_title_label
• etc.

For consistency, these should be in options/locale/locale_en-US.ini (the primary locale file) rather than the custom override.

10. Unclear Error Message

if parsed.form.ForkAndEdit && parsed.form.SubmitChangeRequest {
    ctx.JSONError(ctx.Tr("error.occurred"))
    return
}

Using a generic "error.occurred" message when both flags are set doesn't help debugging. Create a specific message:

ctx.JSONError(ctx.Tr("repo.editor.cannot_fork_and_submit_cr"))

---

Questions

1. Is there intentional logic to allow InternalPush to bypass branch protection for PR submission? If a repository has a protected default branch, shouldn't the change request branch also be subject to protection rules?

2. The AllowNonCollaborator flag in pull service - should there be additional rate limiting or abuse prevention for users who aren't collaborators creating many PRs?

Opus 4.5 with tools:

Details

Code Review: Submit Change Request Feature

1. BUG: Typos in Locale File

issues.ref_pull_from = `<a href="%[3]s">referenced this changchange request %[4]s</a> <a id="%[1]s" href="#%[1]s">%[2]s</a>`
issues.ref_closing_from = `<a href="%[3]s">referenced a changchange request %[4]s that will close this issue</a> <a id="%[1]s" href="#%[1]s">%[2]s</a>`

Issue: The word "changchange" is a typo - it should be "change".

Fix:

issues.ref_pull_from = `<a href="%[3]s">referenced this change request %[4]s</a> <a id="%[1]s" href="#%[1]s">%[2]s</a>`
issues.ref_closing_from = `<a href="%[3]s">referenced a change request %[4]s that will close this issue</a> <a id="%[1]s" href="#%[1]s">%[2]s</a>`

---

2. POTENTIAL RACE CONDITION: Branch Name Generation

func getUniquePatchBranchName(ctx context.Context, prefixName string, repo *repo_model.Repository) string {
	prefix := prefixName + "-patch-"
	for i := 1; i <= maxUniqueNameAttempts; i++ {
		branchName := fmt.Sprintf("%s%d", prefix, i)
		if exist, err := git_model.IsBranchExist(ctx, repo.ID, branchName); err != nil {
			log.Error("getUniquePatchBranchName: %v", err)
			return ""
		} else if !exist {
			return branchName
		}
	}
	return ""
}

Issue: There's a TOCTOU (Time-of-Check-Time-of-Use) race condition. Between checking if the branch exists and actually creating it in handleSubmitChangeRequest, another concurrent request could create the same branch. The concurrent test in submit_change_request_test.go tests different users (user4 and user5), which have different prefixes, but doesn't test the same user submitting concurrently.

Mitigation: The code does handle this gracefully - if the branch creation fails due to conflict, ChangeRepoFiles will return an error and the user will see an error message. However, for better UX, consider:

// Option 1: Add a random suffix to make collisions extremely unlikely
func getUniquePatchBranchName(ctx context.Context, prefixName string, repo *repo_model.Repository) string {
	prefix := prefixName + "-patch-"
	// Add timestamp or random component for uniqueness
	timestamp := time.Now().UnixNano()
	branchName := fmt.Sprintf("%s%d", prefix, timestamp%100000)
	// Then check and increment if needed...
}

// Option 2: Use a database transaction with SELECT FOR UPDATE
// to atomically reserve the branch name

---

3. SECURITY CONSIDERATION: InternalPush Bypasses Hooks

		Signoff:      form.Signoff,
		Author:       parsed.GitCommitter,
		Committer:    parsed.GitCommitter,
		InternalPush: true,

Issue: Using InternalPush: true bypasses pre-receive hooks, which could include security checks like branch protection rules. The comment explains the rationale, but this is a significant security decision.

Recommendation: The code already validates that the user is authenticated and the repository allows PRs. However, ensure that:

1. Branch protection rules for the default branch still apply when the PR is merged
2. Document this behavior clearly for administrators

The current implementation appears acceptable since:

• The branch is created specifically for the PR (not the protected default branch)
• The actual merge still goes through normal merge checks

---

4. BUG: Redundant Repository Open for Cleanup

	gitRepo, err := gitrepo.OpenRepository(ctx, targetRepo)
	if err != nil {
		log.Error("handleSubmitChangeRequest: failed to open git repo: %v", err)
		// Attempt to clean up the orphaned branch - need to open repo specifically for cleanup
		if cleanupRepo, cleanupErr := gitrepo.OpenRepository(ctx, targetRepo); cleanupErr == nil {
			if delErr := repo_service.DeleteBranch(ctx, ctx.Doer, targetRepo, cleanupRepo, branchName, nil); delErr != nil {
				log.Error("handleSubmitChangeRequest: failed to cleanup branch %s: %v", branchName, delErr)
			}
			cleanupRepo.Close()
		}

Issue: If OpenRepository fails, trying to open it again immediately for cleanup will almost certainly fail too. This is dead code that will never succeed.

Fix: Remove the redundant cleanup attempt:

	gitRepo, err := gitrepo.OpenRepository(ctx, targetRepo)
	if err != nil {
		log.Error("handleSubmitChangeRequest: failed to open git repo: %v", err)
		// Note: Cannot clean up branch since we can't open the repository
		// The branch will remain orphaned but this is a rare edge case
		ctx.ServerError("OpenRepository", err)
		return nil
	}

---

5. CSS COMMENTED OUT - Potential UI Issues

/*
.issue-content {
  display: flex;
  align-items: flex-start;
  gap: 16px;
}
...
} */

Issue: Large blocks of CSS are commented out. This could cause layout issues for the issue/PR view pages. If this CSS is no longer needed, it should be removed entirely. If it's temporarily disabled for debugging, it should be restored.

Recommendation: Either remove the commented CSS entirely or restore it if needed for proper layout.

---

6. MISSING INPUT VALIDATION: PR Description Length

	prTitle = util.TruncateRunes(prTitle, 255)
	prContent := strings.TrimSpace(form.ChangeRequestDescription)

Issue: While prTitle is truncated to 255 characters, prContent (the PR description) has no length limit. A malicious user could submit an extremely large description, potentially causing database issues or memory problems.

Fix: Add a reasonable limit:

	prTitle = util.TruncateRunes(prTitle, 255)
	prContent := strings.TrimSpace(form.ChangeRequestDescription)
	// Limit description to a reasonable size (e.g., 65535 characters for TEXT column)
	const maxDescriptionLength = 65535
	prContent = util.TruncateRunes(prContent, maxDescriptionLength)

---

7. IMPROVEMENT: Mutual Exclusivity Check Could Be Earlier

	// Validate mutually exclusive workflow flags
	// Both cannot be true simultaneously - this is a security check in case JavaScript fails
	if parsed.form.ForkAndEdit && parsed.form.SubmitChangeRequest {
		ctx.JSONError(ctx.Tr("error.occurred"))
		return
	}

Issue: This check happens after prepareEditorCommitSubmittedForm which does significant processing. Moving this check earlier would be more efficient.

Recommendation: Consider adding this validation in the form binding layer or at the very start of EditFilePost.

---

8. IMPROVEMENT: Error Message Could Be More Specific

	if parsed.form.ForkAndEdit && parsed.form.SubmitChangeRequest {
		ctx.JSONError(ctx.Tr("error.occurred"))

Issue: Using a generic "error.occurred" message doesn't help users understand what went wrong.

Fix: Add a specific error message:

	if parsed.form.ForkAndEdit && parsed.form.SubmitChangeRequest {
		ctx.JSONError(ctx.Tr("repo.editor.cannot_use_both_fork_and_submit"))

---

9. GOOD PRACTICES OBSERVED

The code demonstrates several good practices:

• Proper branch cleanup on PR creation failure
• Rune-based string truncation for UTF-8 safety
• Comprehensive test coverage including concurrent scenarios
• Clear separation of concerns between fork-and-edit and submit-change-request workflows
• Proper authentication checks before allowing change request submission

---

10. MINOR: Unrelated Change in Diff

-	newNotifyInput(repo, sender, webhook_module.HookEventWorkflowRun).WithPayload(&api.WorkflowRunPayload{
+	newNotifyInput(repo, sender, webhook_module.HookEventWorkflowRun).
+		WithRef(run.Ref).
+		WithPayload(&api.WorkflowRunPayload{

This change adds .WithRef(run.Ref) to the workflow run notification. While this appears to be a bug fix (adding missing ref information), it's unrelated to the change request feature and should ideally be in a separate commit/PR for clarity.

---

Summary

Critical Issues:

1. Typos in locale file ("changchange" → "change")

Medium Issues:
2. Redundant/dead code in repository open failure cleanup
3. Missing length limit on PR description
4. Commented-out CSS that may break layouts

Low/Improvement:
5. Race condition in branch naming (gracefully handled but could be improved)
6. Generic error message for mutually exclusive flags
7. Unrelated change mixed into the PR

[devin-ai-integration]

Devin Review found 1 new potential issue.

View issue and 21 additional flags in Devin Review.

[devin-ai-integration]

🔴 Undefined variable mergeStyleAllowedCount causes merge style dropdown to never render

The Vue template references mergeStyleAllowedCount in a v-if condition, but this variable is never defined in the component's script section.

Click to expand

Analysis

The new code adds a condition to only show the merge style dropdown when multiple styles are available:

<div v-if="mergeStyleAllowedCount > 1" class="ui dropdown icon button" ...>

However, mergeStyleAllowedCount is not defined anywhere in the component - there's no ref, computed property, or data property with this name.

Actual vs Expected

• Actual: mergeStyleAllowedCount is undefined, and undefined > 1 evaluates to false in JavaScript, so the dropdown is never rendered regardless of how many merge styles are available.
• Expected: The dropdown should be hidden only when there's exactly one merge style, and shown when there are multiple styles.

Impact

Users cannot select a different merge style even if the repository is configured to allow multiple merge strategies. While the Forkana-specific configuration only enables squash merge (making this bug's impact minimal in that specific use case), the implementation is broken and would fail silently if additional merge styles were enabled.

Recommendation: Add a computed property to calculate the count of allowed merge styles:

const mergeStyleAllowedCount = computed(() => mergeForm.mergeStyles.filter(s => s.allowed).length);

Or inline the condition: v-if="mergeForm.mergeStyles.filter(s => s.allowed).length > 1"

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[pedrogaudencio]

My AI says No fix is needed - the code is already correct. The mergeStyleAllowedCount variable is declared as a shallowRef(0) at line 24, initialized in onMounted at line 46, and used in the template at line 151.

But probably better for @pieer to confirm.

[pedrogaudencio]

Addressed

• ✅ Typo in Locale File ("changchange")
• ✅ Redundant Cleanup Attempt Will Fail: removed redundant code
• ✅ Test Assumes Specific PR Index: fixed to extracting PR index dynamically from redirect url
• ✅ Duplicate Cleanup Logic Should Be Extracted: extracted to cleanupOrphanedBranch helper function
• ✅ Unclear Error Message: added locale key with specific error message
• ✅ Missing PR Description Length Limit: added util.TruncateRunes(prContent, 65535)

Questions

Is there intentional logic to allow InternalPush to bypass branch protection for PR submission? If a repository has a protected default branch, shouldn't the change request branch also be subject to protection rules?

This is intentional: branch protection applies at merge time, not branch creation.

The AllowNonCollaborator flag in pull service - should there be additional rate limiting or abuse prevention for users who aren't collaborators creating many PRs?

No rate limiting for non-collaborators creating PRs. The existing (Gitea) rate limiting mechanisms apply and specific PR creation limits can always be added later if required.

Skipped

• InternalPush Bypasses Branch Protection: intentional design - middleware verifies permissions
• Missing Repository Archive Check: archive check exists at middleware level via context.RepoMustNotBeArchived() and repo.MustBeEditable
• Title Truncation Edge Case: theoretical edge case - TruncateRunes correctly handles combining characters
• Missing Locale Keys in Options File: in Forkana we use custom/options/locale/ for customisations
• Race Condition in Branch Name Generation: race condition exists in getUniquePatchBranchName(), but it's handled. If collision occurs, ChangeRepoFiles returns an error
• Mutual Exclusivity Check Could Be Earlier: moving check earlier wouldn't be substantially beneficial
• Unrelated Change in Diff: true, unrelated to change request feature but ok to fix here

Not Fixed

(assigning to @pieer)

• CSS Styles Incorrectly Commented Out

[devin-ai-integration]

Devin Review found 1 new potential issue.

View issue and 22 additional flags in Devin Review.

[devin-ai-integration]

Devin Review found 2 new potential issues.

View issues and 27 additional flags in Devin Review.

[devin-ai-integration]

Devin Review found 1 potential issue.

[pedrogaudencio]

@pieer I've checked the issue regarding the CSS styles and reverted them to the original. Instead of commenting the styles completely (as it would break the main templates and only serve the custom ones) I added a new .issue-content-left-pull-request class to force the width of the content to be 100% in case the page open is a PR - this should work well since you removed the sidebar there (so .issue-content-right isn't present anyway and that doesn't affect the width).

@taoeffect after fixing a few other things flagged by Devin, I believe this PR is ready to merge. 👍

[taoeffect]

Thank you @pedrogaudencio!