Skip to content

fix(printer,syncer): address Copilot review feedback from PR #26#27

Merged
takecy merged 6 commits intomasterfrom
fix/printer-stderr-and-cleanup
Apr 26, 2026
Merged

fix(printer,syncer): address Copilot review feedback from PR #26#27
takecy merged 6 commits intomasterfrom
fix/printer-stderr-and-cleanup

Conversation

@takecy
Copy link
Copy Markdown
Owner

@takecy takecy commented Apr 26, 2026

Issue

PR #26 (#26) への GitHub Copilot レビューで挙がった 5 件の指摘を解消する follow-up PR。

Overview

PR #26 は既にマージ済みだが、Copilot レビューが 5 件の改善点を指摘しており、いずれも妥当だったため follow-up として対応する。すべて 1 行〜数行レベルの小修正で、スコープは printer/print.go と syncer/syncer.go / syncer/git_test.go に閉じる。

加えて、コードベース全体に対する AGENTS.md（CLAUDE.md / .github/copilot-instructions.md の symlink 元）の追加を別 worktree から cherry-pick で取り込んだ。

A. stdout / stderr の振り分け修正（b852302）

  • Printer.PrintMsgErr / Printer.PrintRepoErr は名前と赤色テンプレートに反して p.writer (stdout) に書いていた
  • Printer.Error の挙動と整合させ、両者を p.errWriter 経由に変更
  • これにより gih ... 2>err.log >out.log でログ分離が可能に。E2E で stdout に Success/Done/Summary、stderr に Failed/Failed repositories: が振り分けられることを確認

B. dead code の整理（ef5be01）

  • failedRepo.Err フィールドは addFailed で保存される一方、printSummary で参照されない dead field
  • 失敗時のエラー詳細は実行中に既に PrintMsgErr("Failed: %s\n%v", r, err) で stderr に出力済み
  • struct ごと撤廃し failed []string に簡素化。addFailed(r, err) → addFailed(r) で API も縮小

C. 出力フォーマットの整合性（70ccedc）

  • "Success: %s\n" の末尾 \n が msgTpl 末尾の \n と二重で空行を発生させていた
  • Timeout / Failed フォーマットと同じく末尾 \n を削除し 1 行で閉じる形に統一
  • E2E で Success: r1 の直後に空行が無いことを awk で機械的に検証

D. テストの fd leak 対策（f6bea28）

  • TestExistGit_NoStdoutSideEffect の os.Pipe() で r (read 端) が Close されず fd leak
  • defer func() { _ = r.Close() }() を追加（errcheck 回避のため明示的にエラー無視）
  • go test -count=100 で "too many open files" 等が起きないことを確認

E. AGENTS.md の追加（7d420ad）

  • リポジトリガイドラインを AGENTS.md として追加
  • CLAUDE.md と .github/copilot-instructions.md をそれぞれ symlink にして単一の真実の情報源に集約
  • 別 worktree で先行作成していたものを cherry-pick

検証結果

  • make test (-race): 全テスト PASS
  • golangci-lint run: 0 issues
  • make build: 成功
  • E2E: stdout/stderr 分離 / Success 直後の空行解消 / fd leak なし

🤖 Generated with Claude Code

takecy and others added 5 commits April 26, 2026 11:54
@takecy @claude
fix(printer): emit error and repo-list messages on stderr
b852302 
PrintMsgErr and PrintRepoErr were writing to p.writer (stdout)
despite their names and despite the parallel Error method routing
its output to p.errWriter. That meant the gih CLI's error notices
("Failed: <repo>", "Timeout: <repo>") and the final failed/timed-out
repository list landed on stdout — indistinguishable from successful
results when the caller used `2>` to capture errors or `>` to
capture results.

Switch both methods to p.errWriter to match the Error method and the
implicit contract suggested by the *Err naming. Existing tests use
NewPrinter(&buf, &buf) so they keep passing; the new behaviour shows
up only when callers wire distinct writers, which is the use case
this fix enables.

Reported by GitHub Copilot on PR #26.

Co-Authored-By: Claude <noreply@anthropic.com>
@takecy @claude
refactor(syncer): drop unused Err field from failed repo records
ef5be01 
failedRepo carried both the repo path and the error returned by the
git invocation, but the error field was never read after assignment.
The per-repo failure reason is already streamed to stderr at execution
time via PrintMsgErr ("Failed: <repo>\n<err>"), and printSummary only
needs the repo path to print the trailing failed-repo list.

Replace the failedRepo struct with a plain []string and update
addFailed and the summary printer accordingly. The test that asserted
on stats.failed[0].Repo becomes stats.failed[0] — same coverage,
slightly cleaner.

Reported by GitHub Copilot on PR #26.

Co-Authored-By: Claude <noreply@anthropic.com>
@takecy @claude
docs: add AGENTS.md and symlink CLAUDE.md and copilot-instructions.md
7d420ad 
Co-Authored-By: Claude <noreply@anthropic.com>
@takecy @claude
fix(syncer): drop trailing newline from Success message to avoid blan…
70ccedc 
…k line

The Success message used "Success: %s\n" while Timeout and Failed
used the trailing-newline-free form. Combined with msgTpl's own
"\n", the explicit \n in the format string produced an empty line
right after every successful repo. The output already includes a
visible separator from the Print template, so the extra blank line
was pure noise.

Drop the \n from the format string. Output now matches the Timeout
and Failed shapes: a single line ending in the template's trailing
newline.

Reported by GitHub Copilot on PR #26.

Co-Authored-By: Claude <noreply@anthropic.com>
@takecy @claude
test(syncer): close pipe reader to avoid fd leak in TestExistGit_NoSt…
f6bea28 
…doutSideEffect

os.Pipe returns two file descriptors. The test was already closing
the writer to signal EOF for io.ReadAll, but the read end was leaked
on every invocation. One leak per run is harmless on its own; under
go test -count=N or as the suite grows it can exhaust the open-file
ulimit, especially in CI containers with low limits.

Add a deferred Close on the reader. Wrap it in a closure to discard
the error explicitly so the errcheck linter stays happy — the close
result has no diagnostic value in this test, only the fd release
matters.

Reported by GitHub Copilot on PR #26.

Co-Authored-By: Claude <noreply@anthropic.com>
@takecy takecy requested a review from Copilot April 26, 2026 03:06
Copilot AI reviewed Apr 26, 2026
View reviewed changes
Copy link
Copy Markdown

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

Follow-up fixes to address review feedback from PR #26, focusing on output stream correctness (stdout vs stderr), simplifying run failure bookkeeping in the syncer summary, and tightening a small test resource-leak issue. Also adds a repository-wide contributor guideline document (AGENTS.md).

Changes:

  • Route error-oriented printer helpers (PrintMsgErr, PrintRepoErr) to errWriter (stderr) and align output newline behavior.
  • Simplify sync execution stats to store only failed repo paths (removing the unused per-repo error field) and update tests accordingly.
  • Close the read end of an os.Pipe() in a test to avoid FD leaks; add AGENTS.md with repo guidelines.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
printer/print.go Ensures error printing uses errWriter (stderr) and updates related comments.
syncer/syncer.go Removes dead failedRepo error retention, normalizes success output formatting, and simplifies summary printing.
syncer/syncer_test.go Adapts assertions to the simplified failed []string stats structure.
syncer/git_test.go Fixes an FD leak by closing the pipe read end.
AGENTS.md Adds repository guidelines for structure, workflow, testing, and PR conventions.

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

Comment thread AGENTS.md Outdated
@takecy @claude
docs(agents): clarify default-branch wording in worktree workflow
d8f54fb 
Reported by GitHub Copilot on PR #27.

Co-Authored-By: Claude <noreply@anthropic.com>
@takecy takecy marked this pull request as ready for review April 26, 2026 03:12
@takecy takecy merged commit 16458eb into master Apr 26, 2026
2 checks passed
@takecy takecy deleted the fix/printer-stderr-and-cleanup branch April 26, 2026 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@takecy