Conversation
- `HTTPError.SetInternal` to `HTTPError.Wrap.(*HTTPError)` - `HTTPError.WithInternal` to `httpError.Wrap.(*HTTPError)` - Remove `e.Debug` - Fix signature of `HTTPErrorHandler`, `WebhookApplicationsEventHandler` - Use `echo.DefaultHTTPErrorHandler` at once - Use `echo.UnwrapResponse` in `AccessLoggingMiddleware` - `SetParamNames`/`SetParamValues` to `SetPathValues`
Member
Author
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v2 #1009 +/- ##
=====================================
Coverage 7.26% 7.26%
=====================================
Files 118 118
Lines 16913 16931 +18
=====================================
+ Hits 1228 1230 +2
- Misses 15603 15616 +13
- Partials 82 85 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughEcho フレームワークを v4 → v5 に移行し、Go を 1.24.11 → 1.25.0 に更新。複数のハンドラーとミドルウェアで Context パラメータを値型からポインタ型へ変更し、テストのパスパラメータ注入とエラーハンドリングを v5 に合わせて修正した。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/router/middleware.go (1)
36-82:⚠️ Potential issue | 🟡 MinorAccessLoggingMiddleware の
UnwrapResponse失敗時にアクセスログが出力されない。Line 47-52 で
echo.UnwrapResponseが失敗した場合、uErrがそのまま返されますが、以下の問題があります：
- Line 42 で既にエラーハンドラーがレスポンスを書き込んでいる可能性があるため、
uErrを返しても Echo がさらにレスポンスを書こうとする可能性がある- アクセスログが完全にスキップされてしまう
UnwrapResponseが失敗するケースは通常運用では稀ですが、失敗時にもアクセスログ（エラー情報付き）を出力するか、最低限return nilで安全にフォールバックする方が堅牢です。🛡️ 提案: UnwrapResponse 失敗時もログ出力する
rw, uErr := echo.UnwrapResponse(c.Response()) if uErr != nil { logger := logging.GetLogger(req.Context()) logger.Error("failed to unwrap response", zap.Error(uErr)) - return uErr + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/router/middleware.go` around lines 36 - 82, AccessLoggingMiddleware currently returns uErr when echo.UnwrapResponse fails which can cause Echo to attempt another response write and skips access logging; change the unwrap error handling in AccessLoggingMiddleware so that instead of returning uErr you log the unwrap error (include zap.Error(uErr)) and fall back to using c.Response() (or default values) to build and emit the access log (include the original handler error `err` if present), then return nil; update the block that calls echo.UnwrapResponse to emit an error-level log entry for the unwrap failure and continue to the same log-emission switch (server/client/redirect/success) so access logs are always produced.
🧹 Nitpick comments (3)
go.mod (2)
3-3:go 1.25.0への変更はコンテナ環境のGOMAXPROCS挙動に影響します。新しい GOMAXPROCS の挙動（Linux で cgroup CPU 割り当てを考慮）は
go.modにgo 1.25以上が記載されている場合にのみ有効になります。 Kubernetes 等のコンテナ環境では CPU limit に基づいてGOMAXPROCSが自動設定されるため、パフォーマンス特性が変化する可能性があります。意図した変更であることを確認してください。また、Go 1.25.6 は 2026-01-15 にリリースされており、1.25.1 以降のパッチには
net/http等へのセキュリティ修正が含まれています。goディレクティブは最小要件のみを定義するため直接の影響はありませんが、CI/CD やコンテナイメージで使用する実際の Go ツールチェーンが最新パッチ（1.25.6）であることを確認してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 3, go.mod was changed to "go 1.25.0", which alters GOMAXPROCS behavior in containerized environments and may have security/patch implications; decide whether this is intended and either (A) keep it but update the go directive to a patched point release like "go 1.25.6" and ensure your CI/container toolchain images use Go 1.25.6, or (B) revert to the previous major version (e.g., "go 1.24") if you want to preserve prior GOMAXPROCS semantics; update the go directive in go.mod accordingly and adjust CI/build Dockerfiles to match the chosen Go toolchain so runtime/patch behavior is consistent.
15-16: Echo v5 は現時点で本番環境への適用が推奨されていません。Echo 公式リポジトリは「本番環境での使用は 2026-03-31 以降まで待つことを推奨する」と案内しています。 本 PR マージ時点（2026-02-23）はこの期間内であるため、本番デプロイのタイミングを考慮してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 15 - 16, The go.mod currently depends on github.com/labstack/echo/v5 and github.com/labstack/echo-contrib/v5 which Echo maintainers advise against using in production until 2026-03-31; revert these to the stable v4 line by replacing the two v5 module entries with the matching v4 modules (e.g., github.com/labstack/echo/v4 and github.com/labstack/echo-contrib/v4), run go get to update the lock, and run the test suite/build to fix any API differences (search for usages of echo imports in the codebase to adjust import paths/types if needed).internal/router/router.go (1)
27-27: Go命名規則:defaultHttpErrorHandler→defaultHTTPErrorHandlerGoの命名規則では、HTTPのような頭字語は全て大文字にするのが慣例です（例:
HTTPError,HTTPHandler）。HttpではなくHTTPを使用してください。♻️ 修正案
-var defaultHttpErrorHandler = echo.DefaultHTTPErrorHandler(false) +var defaultHTTPErrorHandler = echo.DefaultHTTPErrorHandler(false)Line 36も合わせて変更:
- defaultHttpErrorHandler(c, he) + defaultHTTPErrorHandler(c, he)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/router/router.go` at line 27, Rename the variable defaultHttpErrorHandler to defaultHTTPErrorHandler to follow Go naming conventions for acronyms; update its declaration (var defaultHTTPErrorHandler = echo.DefaultHTTPErrorHandler(false)) and replace all references/usages (e.g., the occurrence mentioned around line 36) to the new identifier so the code compiles and consistently uses defaultHTTPErrorHandler while keeping echo.DefaultHTTPErrorHandler unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/router/middleware.go`:
- Around line 36-82: AccessLoggingMiddleware currently returns uErr when
echo.UnwrapResponse fails which can cause Echo to attempt another response write
and skips access logging; change the unwrap error handling in
AccessLoggingMiddleware so that instead of returning uErr you log the unwrap
error (include zap.Error(uErr)) and fall back to using c.Response() (or default
values) to build and emit the access log (include the original handler error
`err` if present), then return nil; update the block that calls
echo.UnwrapResponse to emit an error-level log entry for the unwrap failure and
continue to the same log-emission switch (server/client/redirect/success) so
access logs are always produced.
---
Nitpick comments:
In `@go.mod`:
- Line 3: go.mod was changed to "go 1.25.0", which alters GOMAXPROCS behavior in
containerized environments and may have security/patch implications; decide
whether this is intended and either (A) keep it but update the go directive to a
patched point release like "go 1.25.6" and ensure your CI/container toolchain
images use Go 1.25.6, or (B) revert to the previous major version (e.g., "go
1.24") if you want to preserve prior GOMAXPROCS semantics; update the go
directive in go.mod accordingly and adjust CI/build Dockerfiles to match the
chosen Go toolchain so runtime/patch behavior is consistent.
- Around line 15-16: The go.mod currently depends on github.com/labstack/echo/v5
and github.com/labstack/echo-contrib/v5 which Echo maintainers advise against
using in production until 2026-03-31; revert these to the stable v4 line by
replacing the two v5 module entries with the matching v4 modules (e.g.,
github.com/labstack/echo/v4 and github.com/labstack/echo-contrib/v4), run go get
to update the lock, and run the test suite/build to fix any API differences
(search for usages of echo imports in the codebase to adjust import paths/types
if needed).
In `@internal/router/router.go`:
- Line 27: Rename the variable defaultHttpErrorHandler to
defaultHTTPErrorHandler to follow Go naming conventions for acronyms; update its
declaration (var defaultHTTPErrorHandler = echo.DefaultHTTPErrorHandler(false))
and replace all references/usages (e.g., the occurrence mentioned around line
36) to the new identifier so the code compiles and consistently uses
defaultHTTPErrorHandler while keeping echo.DefaultHTTPErrorHandler unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
go.modinternal/router/account_manager.gointernal/router/account_manager_test.gointernal/router/application.gointernal/router/application_test.gointernal/router/auth.gointernal/router/error.gointernal/router/file.gointernal/router/file_test.gointernal/router/middleware.gointernal/router/router.gointernal/router/tag.gointernal/router/tag_test.gointernal/router/user.gointernal/router/user_test.gointernal/router/wrapsession/wrapper.gointernal/router/wrapsession/wrapsession.gointernal/traq/traq.gointernal/webhook/webhook.go
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/router/middleware.go (2)
47-53:UnwrapResponse失敗時にアクセスログが完全に欠落します。
UnwrapResponseがエラーを返した場合、そのリクエストのアクセスログ（メソッド、パス、レイテンシ等）が一切出力されません。コメントの意図（アクセスログの失敗はリクエストの失敗ではない）は理解できますが、rwが取得できなくてもリクエスト情報（req.Method,req.URL等）だけは出力するとデバッグに役立ちます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/router/middleware.go` around lines 47 - 53, UnwrapResponse failure currently aborts writing the access log; instead, when echo.UnwrapResponse(c.Response()) returns uErr, still emit the access log using available request fields (e.g., req.Method, req.URL.Path, req.RemoteAddr, req.Header, start/stop times) by obtaining logger := logging.GetLogger(req.Context()) and calling the same access-log path used for the success case with sensible defaults for response-specific fields (status, size, etc. — e.g., status=0 or "-" and size=0) so that requests where rw is unavailable still produce method/path/latency logs for debugging while still logging the unwrap error via logger.Error.
40-53:defaultHTTPErrorHandlerの直接呼び出しとe.HTTPErrorHandlerの関係を確認してください。Line 42 で
defaultHTTPErrorHandlerを直接呼び出していますが、router.goのe.HTTPErrorHandlerには追加のデバッグログ（logger.Debug("handling error", ...)）が含まれています。AccessLoggingMiddlewareは常にnilを返す（Line 81）ため、e.HTTPErrorHandlerが呼ばれることは実質的にありません。現在の設計では、
AccessLoggingMiddleware自身がエラーログを出力するため問題ありませんが、e.HTTPErrorHandlerのカスタムロジック（将来的な追加含む）が bypass される点は認識しておくと良いでしょう。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/router/middleware.go` around lines 40 - 53, AccessLoggingMiddleware currently calls defaultHTTPErrorHandler directly on error (defaultHTTPErrorHandler and AccessLoggingMiddleware), which bypasses the echo instance's configurable e.HTTPErrorHandler and its extra debug logic; change the error path so it delegates to the echo instance's HTTPErrorHandler when present (use e.HTTPErrorHandler(c, err) or the equivalent handler stored on the Echo instance) and fall back to defaultHTTPErrorHandler only if e.HTTPErrorHandler is nil, ensuring any custom/debug logic in e.HTTPErrorHandler is executed while preserving the existing access-log behavior.internal/router/router.go (1)
9-11: Echo v5 安定期間前のアップグレードについて。Echo v5 を本番環境で使用する場合は 2026-03-31 以降にアップグレードすることが推奨されており、それまでは重要な問題に対してセマンティックバージョニングに違反する破壊的 API 変更が行われる可能性があります。
現在（2026-02-23）はこの安定期間の前です。本番デプロイのタイミングやバージョン固定（
v5.0.4）を考慮し、2026-03-31 以降に再度 go.mod のバージョンを確認することをお勧めします。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/router/router.go` around lines 9 - 11, この差分は Echo v5（import: "github.com/labstack/echo/v5" と "github.com/labstack/echo-contrib/v5/session"）を取り込んでいますが、Echo v5 の安定期間前で本番利用にリスクがあるため、go.mod でバージョンを明示的に固定（例: v5.0.4）して依存をピン留めし、router.go の該当 import 周辺に「一時的ピン留め／2026-03-31 以降に再確認」旨の TODO コメントを追加しておいてください; 本番デプロイ前に 2026-03-31 を越えたら go.mod のバージョンを見直し、問題なければアップデートを解除またはバージョンを上げる手順を実行してください。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/router/middleware.go`:
- Around line 47-53: UnwrapResponse failure currently aborts writing the access
log; instead, when echo.UnwrapResponse(c.Response()) returns uErr, still emit
the access log using available request fields (e.g., req.Method, req.URL.Path,
req.RemoteAddr, req.Header, start/stop times) by obtaining logger :=
logging.GetLogger(req.Context()) and calling the same access-log path used for
the success case with sensible defaults for response-specific fields (status,
size, etc. — e.g., status=0 or "-" and size=0) so that requests where rw is
unavailable still produce method/path/latency logs for debugging while still
logging the unwrap error via logger.Error.
- Around line 40-53: AccessLoggingMiddleware currently calls
defaultHTTPErrorHandler directly on error (defaultHTTPErrorHandler and
AccessLoggingMiddleware), which bypasses the echo instance's configurable
e.HTTPErrorHandler and its extra debug logic; change the error path so it
delegates to the echo instance's HTTPErrorHandler when present (use
e.HTTPErrorHandler(c, err) or the equivalent handler stored on the Echo
instance) and fall back to defaultHTTPErrorHandler only if e.HTTPErrorHandler is
nil, ensuring any custom/debug logic in e.HTTPErrorHandler is executed while
preserving the existing access-log behavior.
In `@internal/router/router.go`:
- Around line 9-11: この差分は Echo v5（import: "github.com/labstack/echo/v5" と
"github.com/labstack/echo-contrib/v5/session"）を取り込んでいますが、Echo v5
の安定期間前で本番利用にリスクがあるため、go.mod でバージョンを明示的に固定（例: v5.0.4）して依存をピン留めし、router.go の該当
import 周辺に「一時的ピン留め／2026-03-31 以降に再確認」旨の TODO コメントを追加しておいてください; 本番デプロイ前に
2026-03-31 を越えたら go.mod のバージョンを見直し、問題なければアップデートを解除またはバージョンを上げる手順を実行してください。
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/router/router.go`:
- Around line 32-37: HTTPErrorHandler currently calls logging.GetLogger which
can panic when no logger is in context; change e.HTTPErrorHandler to first
unwrap the response via echo.UnwrapResponse and return immediately if
resp.Committed, then obtain a logger in a nil-safe way (do not call
logging.GetLogger directly) — either check for logger presence (e.g. a
logging.HasLogger/FromContext helper) or fall back to a non-panicking default
logger before calling logger.Debug; keep HTTPErrorHandlerInner and
defaultHTTPErrorHandler usage but remove or guard the debug log since
AccessLoggingMiddleware normally prevents this path.
Comment on lines
+32
to
37
| e.HTTPErrorHandler = func(c *echo.Context, err error) { | ||
| logger := logging.GetLogger(c.Request().Context()) | ||
| logger.Debug("handling error", zap.Error(err)) | ||
| he := HTTPErrorHandlerInner(err) | ||
| c.Echo().DefaultHTTPErrorHandler(he, c) | ||
| defaultHTTPErrorHandler(c, he) | ||
| } |
There was a problem hiding this comment.
logging.GetLogger が panic する潜在リスク
Echo v5 の公式ドキュメントでは、カスタムエラーハンドラ内でまず echo.UnwrapResponse で resp.Committed を確認する実装パターンが推奨されています。
それとは別に、Line 33 の logging.GetLogger は context にロガーが存在しない場合に panic します（internal/logging/context.go の実装による）。
e.HTTPErrorHandler が呼ばれるのはミドルウェアチェーンがエラーを返したときですが、AccessLoggingMiddleware はハンドラからのエラーを全て吸収して nil を返します。そのため、このハンドラが実際に呼ばれるのは middleware.RequestID() や setLoggerMiddleware でエラーが発生した場合のみです。setLoggerMiddleware が実行される前（RequestID のエラー時）は context にロガーが存在せず、panic が発生します。
🛡️ 防御的実装への変更案
e.HTTPErrorHandler = func(c *echo.Context, err error) {
- logger := logging.GetLogger(c.Request().Context())
+ logger, logErr := logging.GetLoggerMaybe(c.Request().Context())
+ if logErr != nil {
+ logger = zap.NewNop()
+ }
logger.Debug("handling error", zap.Error(err))また、AccessLoggingMiddleware がエラーを全て吸収するため、Line 34 の logger.Debug("handling error", ...) は通常のリクエストでは実質的に到達しないコードになっています。デバッグログの必要性についても確認を推奨します。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/router/router.go` around lines 32 - 37, HTTPErrorHandler currently
calls logging.GetLogger which can panic when no logger is in context; change
e.HTTPErrorHandler to first unwrap the response via echo.UnwrapResponse and
return immediately if resp.Committed, then obtain a logger in a nil-safe way (do
not call logging.GetLogger directly) — either check for logger presence (e.g. a
logging.HasLogger/FromContext helper) or fall back to a non-panicking default
logger before calling logger.Debug; keep HTTPErrorHandlerInner and
defaultHTTPErrorHandler usage but remove or guard the debug log since
AccessLoggingMiddleware normally prevents this path.
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.
Summary by CodeRabbit
Chores
Bug Fixes