Feat/fix 018

[JonghunYu]

• auto increment counter if the number is already occupied

Summary by CodeRabbit

• Bug Fixes

  • ID assignment now automatically skips IDs already in use, avoiding conflicts and correctly handling gaps.

• Tests

  • Added tests covering ID assignment with occupied IDs and gaps to ensure counter updates and behavior.

• Chores

  • Incremented stored issue counter and added test/ to ignored paths.

• Documentation

  • Minor formatting tweaks and added a manual test instruction for the test/ directory.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The PR increments the counter file from 18 to 19 and changes ID allocation logic: GetNextID() in pkg/storage.go now scans from the current counter, skips IDs that already exist in open or closed directories, returns the first unused ID, and updates the counter to one past the assigned ID. Two unit tests were added in pkg/storage_test.go to validate skipping occupied IDs and handling gaps. .gitignore now ignores test/, and minor formatting/manual-test notes were added to CLAUDE.md.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Review loop and existence checks in pkg/storage.go for correctness and off-by-one behavior
• Verify counter file write/update logic matches intended semantics (writes availableID+1)
• Run and inspect the two new tests in pkg/storage_test.go for coverage and flakiness
• Quick check of .issues/.counter increment and .gitignore/CLAUDE.md non-functional edits

Poem

🐇 I nibble gaps and count with care,
Hopping past IDs already there,
From open nests to closed-up lairs,
I find a number free and fair,
Then thump the ground — the next one’s spared!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check	❓ Inconclusive	The title 'Feat/fix 018' is vague and uses generic terms that don't clearly convey the specific change, making it unclear to teammates scanning history.	Improve the title to be more descriptive, such as 'Auto-increment counter to skip occupied IDs' or 'Implement ID skipping logic in GetNextID()' to better reflect the actual change.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/fix-018

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 20f3565 and c2acf8c.

📒 Files selected for processing (2)

• .gitignore (1 hunks)
• CLAUDE.md (3 hunks)

✅ Files skipped from review due to trivial changes (1)

• CLAUDE.md

🔇 Additional comments (1)

.gitignore (1)

39-39: LGTM!

Adding test/ to the ignore list is reasonable for excluding temporary test directories and artifacts, consistent with the existing patterns for coverage and build outputs.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

pkg/storage.go (1)

97-132: Implementation looks solid—correctly skips occupied IDs and fills gaps.

The loop-based approach to find the next available ID is straightforward and handles both skipping occupied IDs and filling gaps in the sequence. The logic correctly:

• Iterates from the current counter value until an available ID is found
• Updates the counter to availableID + 1
• Returns the first available ID

The implementation aligns well with the test coverage in storage_test.go.

Optional: Consider adding a safeguard against unbounded iteration.

While unlikely in practice, if there are a very large number of consecutive occupied IDs (e.g., due to manual counter manipulation), the loop could iterate many times. Consider adding a maximum iteration limit or logging a warning after a certain threshold.

Example safeguard:

 	// Find the next available ID by checking if current ID exists
 	availableID := currentID
+	maxIterations := 10000
+	iterations := 0
 	for {
+		if iterations >= maxIterations {
+			return 0, fmt.Errorf("too many occupied IDs; exhausted search after %d iterations", maxIterations)
+		}
 		formattedID := FormatID(availableID)
 		_, _, err := FindIssueFile(formattedID)
 		if err != nil {
 			// ID not found, so it's available
 			break
 		}
 		// ID exists (in either open or closed), try next one
 		availableID++
+		iterations++
 	}

.issues/open/018-increment-counter.md (1)

16-19: Specify language for fenced code block.

The fenced code block should specify a language identifier (e.g., bash or shell) for better rendering and to satisfy the markdownlint rule MD040.

Apply this diff:

-```
+```bash
 $ gi create aa
 Error: failed to save issue: issue 003 exists in closed directory, cannot save to open

</blockquote></details>

</blockquote></details>

<details>
<summary>📜 Review details</summary>

**Configuration used**: Path: .coderabbit.yaml

**Review profile**: CHILL

**Plan**: Pro

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between d5614077260fb36e9f04ba12ad8ded100666bd20 and 20f3565a566a7fcef48adf68fa3a6d3b992964c4.

</details>

<details>
<summary>📒 Files selected for processing (4)</summary>

* `.issues/.counter` (1 hunks)
* `.issues/open/018-increment-counter.md` (1 hunks)
* `pkg/storage.go` (2 hunks)
* `pkg/storage_test.go` (1 hunks)

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>🧬 Code graph analysis (2)</summary>

<details>
<summary>pkg/storage.go (1)</summary><blockquote>

<details>
<summary>pkg/parser.go (1)</summary>

* `FormatID` (108-110)

</details>

</blockquote></details>
<details>
<summary>pkg/storage_test.go (1)</summary><blockquote>

<details>
<summary>pkg/storage.go (8)</summary>

* `InitializeRepo` (22-95)
* `NewIssue` (382-397)
* `SaveIssue` (135-163)
* `OpenDir` (14-14)
* `ClosedDir` (15-15)
* `IssuesDir` (13-13)
* `CounterFile` (16-16)
* `GetNextID` (98-132)

</details>

</blockquote></details>

</details><details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>

<details>
<summary>.issues/open/018-increment-counter.md</summary>

16-16: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</details>

</details>

</details>

<details>
<summary>🔇 Additional comments (3)</summary><blockquote>

<details>
<summary>pkg/storage_test.go (2)</summary><blockquote>

`119-165`: **Excellent test coverage for occupied ID skipping.**

This test thoroughly validates that `GetNextID()` correctly skips occupied IDs (1, 2, 3) across both open and closed directories, returns the next available ID (4), and updates the counter appropriately (5).

---

`167-222`: **Excellent test coverage for gap-filling behavior.**

This test comprehensively validates that `GetNextID()` correctly identifies and fills gaps in the ID sequence. The three successive calls verify:
1. Finding the first gap (ID 2)
2. Finding the second gap (ID 4)
3. Continuing past the last occupied ID (returns 6 after 5)

This ensures the implementation handles sparse ID sequences correctly.

</blockquote></details>
<details>
<summary>.issues/.counter (1)</summary><blockquote>

`1-1`: **Counter progression looks correct.**

The increment from 18 to 19 is consistent with the creation of issue 018 documented in this PR.

</blockquote></details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->