Repository navigation
feat: forgejo issue write ops — create with labels, comment, comments, close zombie check (#16) - #19
Closed
four-bytes-robby wants to merge 2 commits into
Closed
four-bytes-robby wants to merge 2 commits into
four-bytes-robby wants to merge 2 commits into
Conversation
…, close zombie check #16 - forgejo_issue_create: label names → ids (repo + org) before POST; unknown label aborts, nothing created - forgejo_issue_comment / forgejo_issue_comments (one line per comment) - forgejo_issue_close: comment now optional; reports PRs referencing the issue - fix: getForgejoRepo parsed https remotes as host/owner/repo — every forgejo_* tool failed on https origins; now repoFromRemoteUrl (https, ssh, scp, subpath) - forgejoApiAll pagination helper; shared fetch-mock test helpers - Tool count 22 → 25, docs updated, 0.3.0
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…mbie scan 500 PRs #16 - forgejoApiAll: a failed later page fails the call instead of returning a partial list that looks complete - resolveLabelIds: only a 404 on org labels means "no org"; other failures are reported instead of calling org labels unknown - forgejo_issue_close: zombie check scans 500 PRs and says so when none match - tests: pagination, failed page, org-label errors, API error paths, comment-then-close, not-configured for every new tool
four-bytes-robby
marked this pull request as ready for review
October 1, 2026 12:07
There was a problem hiding this comment.
11 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tools/forgejo-issue-create.ts">
<violation number="1" location="src/tools/forgejo-issue-create.ts:20">
P3: The unknown-label error points users to `rollout-defekt-labels --apply`, but this package does not provide or document that command. Replace the repository-specific instruction with a valid Forgejo label-creation direction or a generic instruction to create the labels first.</violation>
</file>
<file name="src/tools/forgejo-issue-comments.ts">
<violation number="1" location="src/tools/forgejo-issue-comments.ts:71">
P1: This request reads only the first paginated comments page, so `slice(-limit)` returns the newest comments from that page rather than the newest comments overall once an issue exceeds one page. Fetch the pages with `forgejoApiAll` (and propagate page failures) before formatting.</violation>
</file>
<file name="tests/forgejo-issue-write.test.ts">
<violation number="1" location="tests/forgejo-issue-write.test.ts:55">
P3: This test claims to verify that a repo label wins over an org label, but `matchLabelIds` has no repo/org concept — it only takes the first entry in the array (via `[...available].reverse()`). The repo-over-org preference lives in `resolveLabelIds`'s array ordering, so this test passes unchanged even if `resolveLabelIds` swapped the order and org labels won. Reword the name to say it asserts first-in-list wins, and add a `resolveLabelIds` test that passes both a repo labels list and an org labels list to actually cover the preference.</violation>
</file>
<file name="src/lib/forgejo-utils.ts">
<violation number="1" location="src/lib/forgejo-utils.ts:247">
P2: This rejects valid scp-style remotes without an explicit username, such as `forgejo.example.com:acme/widgets.git`. Allow the optional `user@` prefix so those repositories do not fail repository detection.</violation>
<violation number="2" location="src/lib/forgejo-utils.ts:251">
P3: `repoFromRemoteUrl` does not strip `.git` when the remote URL has a trailing slash: `https://host/acme/widgets.git/` returns `acme/widgets.git`, and every subsequent `forgejo_*` API call then targets a repo named `widgets.git` and fails with a confusing 404. Strip trailing slashes before removing the suffix.</violation>
<violation number="3" location="src/lib/forgejo-utils.ts:328">
P2: The default 10-page cap silently truncates label lookup at 500 labels. Remove the cap for label resolution or return an explicit truncation error instead of aborting creation as an unknown-label failure.</violation>
</file>
<file name="tests/forgejo-helpers.ts">
<violation number="1" location="tests/forgejo-helpers.ts:25">
P3: `exampleRepo()` creates a temp git repo that is never removed — every `bun test` run leaves a `git-forgejo-*` dir (with `.git`) in the OS tmpdir, accumulating over time. The repo already shows the cleanup convention in `tests/git-status.test.ts` (`rmSync(dir, { recursive: true, force: true })` in a `finally`).</violation>
<violation number="2" location="tests/forgejo-helpers.ts:26">
P3: `exampleRepo()` ignores the exit codes of `git init` and `git remote add origin`. If either fails (missing git, service failure), the fixture still returns a directory that is not the expected repo, and the failing tests then report misleading `no origin remote` / `No Forgejo token` errors from the tools instead of the fixture failure. Check `exitCode` on both spawns and throw with stderr.</violation>
<violation number="3" location="tests/forgejo-helpers.ts:51">
P3: A route answering `{ status: 204 }` (or 205/304) makes this line throw: per the Fetch spec the Response constructor rejects a body on those null-body statuses, so the mock fails with a TypeError instead of reaching the tool's status handling. Build the Response without a body for those statuses.</violation>
</file>
<file name="src/tools/forgejo-issue-comment.ts">
<violation number="1" location="src/tools/forgejo-issue-comment.ts:33">
P3: This new tool duplicates the inline comment POST already in forgejo-issue-close (step 2), and the two have already diverged: the new tool rejects empty/whitespace bodies, while the close tool's `if (comment)` posts a whitespace-only body. Extract a shared `postForgejoComment(repo, issue, body, config)` helper in forgejo-utils and call it from both tools so the handling stays consistent.</violation>
</file>
<file name="src/tools/forgejo-issue-close.ts">
<violation number="1" location="src/tools/forgejo-issue-close.ts:70">
P3: The scan window of the 500 most-recently-updated PRs systematically misses the classic zombie case: a PR merged long ago whose issue stayed open has an old `updated_unix` and drops out of the top 500 on any active repo, so the check reports `No PR references #issue` precisely when the zombie PR exists. The message discloses the window, but the tool-level conclusion it feeds ('closing without a linked merge') is then misleading for old issues.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (!repo) return 'Could not determine Forgejo repository from origin remote.'; | ||
|
|
||
| // This endpoint is not paginated — it returns every comment in one response. | ||
| const result = await forgejoApi(`/repos/${repo}/issues/${issueNum}/comments`, cfg.config); |
There was a problem hiding this comment.
P1: This request reads only the first paginated comments page, so slice(-limit) returns the newest comments from that page rather than the newest comments overall once an issue exceeds one page. Fetch the pages with forgejoApiAll (and propagate page failures) before formatting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/tools/forgejo-issue-comments.ts, line 71:
<comment>This request reads only the first paginated comments page, so `slice(-limit)` returns the newest comments from that page rather than the newest comments overall once an issue exceeds one page. Fetch the pages with `forgejoApiAll` (and propagate page failures) before formatting.</comment>
<file context>
@@ -0,0 +1,82 @@
+ if (!repo) return 'Could not determine Forgejo repository from origin remote.';
+
+ // This endpoint is not paginated — it returns every comment in one response.
+ const result = await forgejoApi(`/repos/${repo}/issues/${issueNum}/comments`, cfg.config);
+ if (!result.ok) return `Error reading comments on #${issueNum}: ${result.error}`;
+
</file context>
| export async function forgejoApiAll( | ||
| path: string, | ||
| config: ForgejoConfig, | ||
| maxPages = 10 |
There was a problem hiding this comment.
P2: The default 10-page cap silently truncates label lookup at 500 labels. Remove the cap for label resolution or return an explicit truncation error instead of aborting creation as an unknown-label failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/lib/forgejo-utils.ts, line 328:
<comment>The default 10-page cap silently truncates label lookup at 500 labels. Remove the cap for label resolution or return an explicit truncation error instead of aborting creation as an unknown-label failure.</comment>
<file context>
@@ -296,3 +316,97 @@ export async function forgejoApi(
+export async function forgejoApiAll(
+ path: string,
+ config: ForgejoConfig,
+ maxPages = 10
+): Promise<ForgejoApiResult> {
+ const sep = path.includes('?') ? '&' : '?';
</file context>
Suggested change
| maxPages = 10 | |
| maxPages = Number.POSITIVE_INFINITY |
| return null; | ||
| } | ||
| } else { | ||
| const scp = trimmed.match(/^[^@]+@[^:]+:(.+)$/); |
There was a problem hiding this comment.
P2: This rejects valid scp-style remotes without an explicit username, such as forgejo.example.com:acme/widgets.git. Allow the optional user@ prefix so those repositories do not fail repository detection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/lib/forgejo-utils.ts, line 247:
<comment>This rejects valid scp-style remotes without an explicit username, such as `forgejo.example.com:acme/widgets.git`. Allow the optional `user@` prefix so those repositories do not fail repository detection.</comment>
<file context>
@@ -228,18 +228,38 @@ export function getForgejoConfig(
+ return null;
+ }
+ } else {
+ const scp = trimmed.match(/^[^@]+@[^:]+:(.+)$/);
+ if (!scp) return null;
+ path = scp[1]!;
</file context>
Suggested change
| const scp = trimmed.match(/^[^@]+@[^:]+:(.+)$/); | |
| const scp = trimmed.match(/^(?:[^@]+@)?[^:]+:(.+)$/); |
|
|
||
| /** Refusal line for unknown labels. Pure — exported for testing. */ | ||
| export function formatUnknownLabels(unknown: string[], repo: string): string { | ||
| return `✗ Issue not created — unknown label(s) in ${repo}: ${unknown.join(', ')}. Create them first (e.g. rollout-defekt-labels --apply), then retry.`; |
There was a problem hiding this comment.
P3: The unknown-label error points users to rollout-defekt-labels --apply, but this package does not provide or document that command. Replace the repository-specific instruction with a valid Forgejo label-creation direction or a generic instruction to create the labels first.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/tools/forgejo-issue-create.ts, line 20:
<comment>The unknown-label error points users to `rollout-defekt-labels --apply`, but this package does not provide or document that command. Replace the repository-specific instruction with a valid Forgejo label-creation direction or a generic instruction to create the labels first.</comment>
<file context>
@@ -0,0 +1,85 @@
+
+/** Refusal line for unknown labels. Pure — exported for testing. */
+export function formatUnknownLabels(unknown: string[], repo: string): string {
+ return `✗ Issue not created — unknown label(s) in ${repo}: ${unknown.join(', ')}. Create them first (e.g. rollout-defekt-labels --apply), then retry.`;
+}
+
</file context>
Suggested change
| return `✗ Issue not created — unknown label(s) in ${repo}: ${unknown.join(', ')}. Create them first (e.g. rollout-defekt-labels --apply), then retry.`; | |
| return `✗ Issue not created — unknown label(s) in ${repo}: ${unknown.join(', ')}. Create the labels first, then retry.`; |
| }); | ||
|
|
||
| it('prefers a repo label over an org label of the same name', () => { | ||
| expect(matchLabelIds(['bug'], [{ id: 1, name: 'bug' }, { id: 50, name: 'bug' }]).ids).toEqual([1]); |
There was a problem hiding this comment.
P3: This test claims to verify that a repo label wins over an org label, but matchLabelIds has no repo/org concept — it only takes the first entry in the array (via [...available].reverse()). The repo-over-org preference lives in resolveLabelIds's array ordering, so this test passes unchanged even if resolveLabelIds swapped the order and org labels won. Reword the name to say it asserts first-in-list wins, and add a resolveLabelIds test that passes both a repo labels list and an org labels list to actually cover the preference.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/forgejo-issue-write.test.ts, line 55:
<comment>This test claims to verify that a repo label wins over an org label, but `matchLabelIds` has no repo/org concept — it only takes the first entry in the array (via `[...available].reverse()`). The repo-over-org preference lives in `resolveLabelIds`'s array ordering, so this test passes unchanged even if `resolveLabelIds` swapped the order and org labels won. Reword the name to say it asserts first-in-list wins, and add a `resolveLabelIds` test that passes both a repo labels list and an org labels list to actually cover the preference.</comment>
<file context>
@@ -0,0 +1,384 @@
+ });
+
+ it('prefers a repo label over an org label of the same name', () => {
+ expect(matchLabelIds(['bug'], [{ id: 1, name: 'bug' }, { id: 50, name: 'bug' }]).ids).toEqual([1]);
+ });
+});
</file context>
Comment on lines
+26
to
+27
| Bun.spawnSync(['git', 'init', '-q'], { cwd: dir }); | ||
| Bun.spawnSync(['git', 'remote', 'add', 'origin', `${HOST}/acme/widgets.git`], { cwd: dir }); |
There was a problem hiding this comment.
P3: exampleRepo() ignores the exit codes of git init and git remote add origin. If either fails (missing git, service failure), the fixture still returns a directory that is not the expected repo, and the failing tests then report misleading no origin remote / No Forgejo token errors from the tools instead of the fixture failure. Check exitCode on both spawns and throw with stderr.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/forgejo-helpers.ts, line 26:
<comment>`exampleRepo()` ignores the exit codes of `git init` and `git remote add origin`. If either fails (missing git, service failure), the fixture still returns a directory that is not the expected repo, and the failing tests then report misleading `no origin remote` / `No Forgejo token` errors from the tools instead of the fixture failure. Check `exitCode` on both spawns and throw with stderr.</comment>
<file context>
@@ -0,0 +1,65 @@
+/** A git repo with `origin` at the example host. */
+export function exampleRepo(): string {
+ const dir = mkdtempSync(join(tmpdir(), 'git-forgejo-'));
+ Bun.spawnSync(['git', 'init', '-q'], { cwd: dir });
+ Bun.spawnSync(['git', 'remote', 'add', 'origin', `${HOST}/acme/widgets.git`], { cwd: dir });
+ return dir;
</file context>
Suggested change
| Bun.spawnSync(['git', 'init', '-q'], { cwd: dir }); | |
| Bun.spawnSync(['git', 'remote', 'add', 'origin', `${HOST}/acme/widgets.git`], { cwd: dir }); | |
| const init = Bun.spawnSync(['git', 'init', '-q'], { cwd: dir }); | |
| if (init.exitCode !== 0) throw new Error(`git init failed: ${init.stderr.toString()}`); | |
| const remote = Bun.spawnSync(['git', 'remote', 'add', 'origin', `${HOST}/acme/widgets.git`], { cwd: dir }); | |
| if (remote.exitCode !== 0) throw new Error(`git remote add failed: ${remote.stderr.toString()}`); |
| if (!scp) return null; | ||
| path = scp[1]!; | ||
| } | ||
| const parts = path.replace(/\.git$/, '').split('/').filter((p) => p !== ''); |
There was a problem hiding this comment.
P3: repoFromRemoteUrl does not strip .git when the remote URL has a trailing slash: https://host/acme/widgets.git/ returns acme/widgets.git, and every subsequent forgejo_* API call then targets a repo named widgets.git and fails with a confusing 404. Strip trailing slashes before removing the suffix.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/lib/forgejo-utils.ts, line 251:
<comment>`repoFromRemoteUrl` does not strip `.git` when the remote URL has a trailing slash: `https://host/acme/widgets.git/` returns `acme/widgets.git`, and every subsequent `forgejo_*` API call then targets a repo named `widgets.git` and fails with a confusing 404. Strip trailing slashes before removing the suffix.</comment>
<file context>
@@ -228,18 +228,38 @@ export function getForgejoConfig(
+ if (!scp) return null;
+ path = scp[1]!;
+ }
+ const parts = path.replace(/\.git$/, '').split('/').filter((p) => p !== '');
+ if (parts.length < 2) return null;
+ // Forgejo API paths take raw owner/repo — do NOT encodeURIComponent.
</file context>
Suggested change
| const parts = path.replace(/\.git$/, '').split('/').filter((p) => p !== ''); | |
| const parts = path.replace(/\/+$/, '').replace(/\.git$/, '').split('/').filter((p) => p !== ''); |
|
|
||
| /** A git repo with `origin` at the example host. */ | ||
| export function exampleRepo(): string { | ||
| const dir = mkdtempSync(join(tmpdir(), 'git-forgejo-')); |
There was a problem hiding this comment.
P3: exampleRepo() creates a temp git repo that is never removed — every bun test run leaves a git-forgejo-* dir (with .git) in the OS tmpdir, accumulating over time. The repo already shows the cleanup convention in tests/git-status.test.ts (rmSync(dir, { recursive: true, force: true }) in a finally).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/forgejo-helpers.ts, line 25:
<comment>`exampleRepo()` creates a temp git repo that is never removed — every `bun test` run leaves a `git-forgejo-*` dir (with `.git`) in the OS tmpdir, accumulating over time. The repo already shows the cleanup convention in `tests/git-status.test.ts` (`rmSync(dir, { recursive: true, force: true })` in a `finally`).</comment>
<file context>
@@ -0,0 +1,65 @@
+
+/** A git repo with `origin` at the example host. */
+export function exampleRepo(): string {
+ const dir = mkdtempSync(join(tmpdir(), 'git-forgejo-'));
+ Bun.spawnSync(['git', 'init', '-q'], { cwd: dir });
+ Bun.spawnSync(['git', 'remote', 'add', 'origin', `${HOST}/acme/widgets.git`], { cwd: dir });
</file context>
| const repo = await getForgejoRepo(cwd); | ||
| if (!repo) return 'Could not determine Forgejo repository from origin remote.'; | ||
|
|
||
| const result = await forgejoApi(`/repos/${repo}/issues/${issueNum}/comments`, cfg.config, { |
There was a problem hiding this comment.
P3: This new tool duplicates the inline comment POST already in forgejo-issue-close (step 2), and the two have already diverged: the new tool rejects empty/whitespace bodies, while the close tool's if (comment) posts a whitespace-only body. Extract a shared postForgejoComment(repo, issue, body, config) helper in forgejo-utils and call it from both tools so the handling stays consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/tools/forgejo-issue-comment.ts, line 33:
<comment>This new tool duplicates the inline comment POST already in forgejo-issue-close (step 2), and the two have already diverged: the new tool rejects empty/whitespace bodies, while the close tool's `if (comment)` posts a whitespace-only body. Extract a shared `postForgejoComment(repo, issue, body, config)` helper in forgejo-utils and call it from both tools so the handling stays consistent.</comment>
<file context>
@@ -0,0 +1,47 @@
+ const repo = await getForgejoRepo(cwd);
+ if (!repo) return 'Could not determine Forgejo repository from origin remote.';
+
+ const result = await forgejoApi(`/repos/${repo}/issues/${issueNum}/comments`, cfg.config, {
+ method: 'POST',
+ body: { body },
</file context>
| const list = linked.map((p) => `!${p.number} (${p.state})`).join(', '); | ||
| return `⚠ No merged PR for #${issue} — referenced by ${list}. Closing as requested.`; | ||
| } | ||
| return `⚠ No PR references #${issue} (checked the ${LINKED_PULL_SCAN} most recently updated PRs) — closing without a linked merge.`; |
There was a problem hiding this comment.
P3: The scan window of the 500 most-recently-updated PRs systematically misses the classic zombie case: a PR merged long ago whose issue stayed open has an old updated_unix and drops out of the top 500 on any active repo, so the check reports No PR references #issue precisely when the zombie PR exists. The message discloses the window, but the tool-level conclusion it feeds ('closing without a linked merge') is then misleading for old issues.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/tools/forgejo-issue-close.ts, line 70:
<comment>The scan window of the 500 most-recently-updated PRs systematically misses the classic zombie case: a PR merged long ago whose issue stayed open has an old `updated_unix` and drops out of the top 500 on any active repo, so the check reports `No PR references #issue` precisely when the zombie PR exists. The message discloses the window, but the tool-level conclusion it feeds ('closing without a linked merge') is then misleading for old issues.</comment>
<file context>
@@ -21,6 +22,52 @@ export interface ForgejoCloseResult {
+ const list = linked.map((p) => `!${p.number} (${p.state})`).join(', ');
+ return `⚠ No merged PR for #${issue} — referenced by ${list}. Closing as requested.`;
+ }
+ return `⚠ No PR references #${issue} (checked the ${LINKED_PULL_SCAN} most recently updated PRs) — closing without a linked merge.`;
}
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #16 · stacked on #18 (base retargets to main once #18 merges)
forgejo_issue_create: label names → ids (repo + org) before POST; unknown label aborts, nothing createdforgejo_issue_comment/forgejo_issue_comments(one line per comment, newest N)forgejo_issue_close: comment arg now optional; reports PRs referencing the issuegetForgejoRepoparsed https remotes ashost/owner/repo— everyforgejo_*tool failed on https origins; replaced byrepoFromRemoteUrlGates: tsc clean · 279 tests pass · build ok · local review 95% (after fix commit 57e76ef: no partial pages, org-label errors surface, zombie scan 500 PRs, error-path tests)
Summary by cubic
Adds Forgejo issue write tools — create with labels, comment, and list comments — plus a zombie check on close, and fixes the remote parsing bug that broke every forgejo_* tool on https origins.
forgejo_issue_createresolves label names against repo and org labels before POSTing; unknown labels abort with nothing created.forgejo_issue_commentsshows one line per comment with newest N;forgejo_issue_closemakes the comment optional and reports linked PRs referencing the issue (scans the 500 most recently updated).repoFromRemoteUrlhandles https, ssh, scp-style, and subpath remotes. Pagination and label failures now surface: a failed page fails the whole list call, and only a 404 on org labels counts as "no org".Written for commit 57e76ef. Summary will update on new commits.