BE/FE refact#254
Merged
Merged
Conversation
Refactor/backend/deadcode
📝 WalkthroughWalkthroughThis PR tightens backend task handler error contracts (raising NonRetryableError for missing payload/state), centralizes environment key usage, refactors frontend auth/session and form state into hooks, centralizes API paths and task-status utilities, adds duplication-detection tooling and CLAUDE skill/docs, and consolidates Terraform environment stacks into a shared devforge_stack with monitoring modules and moved state mappings. ChangesBackend Task Handler Error Contracts & Env Keys
Frontend Refactoring: Auth, Hooks, API Paths, Utilities, Tests
Tooling, Docs, and Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Owner
Author
Frontend Refactor: 責務分離・契約集約・race 防止テストの整備Summary
Backgroundfrontend のリファクタリングレビューで次の点を High/Medium として挙げた:
ChangesStep 1: ProjectModal →
|
| ファイル | ケース | 守る仕様 |
|---|---|---|
useDocumentForm.test.ts |
Redux キャッシュ存在時に loadLatest が呼ばれない |
ページ遷移時の二重 fetch / チラつき防止 |
useAsyncAnalysisPage.test.ts |
fetchProgress が reject しても polling フェーズが維持される |
Redis 障害時に hook 全体が壊れない |
useAuthSession.test.ts（新規, 5 ケース） |
sessionStorage 復元 / 公開パス / 保護パスでの復元 / ログアウト後の race 防止 / handleLoginSuccess | ログアウト直後の getCurrentUser 競合を防ぐ（従来 App.tsx でテスト不能だった経路） |
useProjectModalForm.test.ts（新規, 10 ケース） |
初期化 / structuredClone / is_current 切替 / techStack / teamMember / phase / dateError | hook 抽出後の updater ロジック単体テスト |
整理（低価値テストの削減）
components/analysis/FrameworkList.test.tsx→TechBar.test.tsxにリネーム（実態が TechBar なのにファイル名が乖離していた）。// items を使ってESLint の unused variable を回避の dummy assert を撤去。
レビュー誤検出の訂正
レビュー時に「不足」と挙げた 2 件は 既存テストでカバー済み だったため追加せず:
- 並列 refresh 抑止 →
api/client.test.ts:71-107に既存 useBlogAccountManagerの sync 失敗 →useBlogAccountManager.test.ts:160-186に既存
Validation
make lint-frontend # All checks passed
make test-frontend # 17 files, 92 tests pass（前回 15 files, 75 tests → +17 ケース）
make build-frontend # 成功（既存の chunk size warning のみ）
npm run test:e2e # 13/13 pass（auth / navigation / notifications / github-analysis）App.tsx の認証ライフサイクル変更は CLAUDE.md の E2E トリガー（「認証・ナビゲーション・レイアウトの変更」）に該当するため Playwright も実行済み。
Test plan
-
make ci相当（lint / test / build）すべて pass -
npm run test:e2e13 シナリオすべて pass -
useAuthSessionrace 防止テスト（ログアウト後にgetCurrentUserが呼ばれない）が新規で追加 -
useProjectModalFormの 10 updater 分岐がすべてテスト済み - ブラウザでの手動確認（任意）:
- 職務経歴書 → プロジェクト編集モーダルで保存/キャンセル/技術スタック追加削除
- ブログ連携で zenn / note / qiita の保存 → 自動同期 → 同期失敗時のメッセージ表示
- ログアウト → 再ログイン直後にちらつきが無いこと
Impact / Risk
- 挙動変更なし: すべて内部リファクタリングで、UI / API 通信 / Redux 状態の外部観測動作は維持。
- 追加ガード:
useAuthSessionテストで race 経路を、useDocumentFormテストでキャッシュ復帰経路をそれぞれ守るようになった。今回見つけたわけではないが、潜在的回帰の検知力が上がる。 - コード行数: 削除 -352 行 / 追加 +247 行（差し引き -105 行、テスト追加分含む）。
Out of scope（別 PR で対応予定）
frontend リファクタリングレビューで挙げた以下は本 PR では未対応:
hooks/をhooks/auth/hooks/form/hooks/blog/等のサブディレクトリへ再編（import 影響が広範のため別 PR）useDocumentForm内の Redux キャッシュ wiring をuseFormCacheに分離useCareerAnalysisPage:37のpollingId!non-null assertion 除去GitHubAnalysisPage.tsxのダッシュボード JSX 87 行を子コンポーネントへ抽出CareerResumeForm.tsxのヘッダーアクション群を<CareerResumeFormHeader>に切り出しpages/GitHubCallbackPage.tsxを thin wrapper 化（実体をcomponents/auth/へ）
🤖 Generated with Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
frontend/src/hooks/useProjectModalForm.test.ts (1)
22-108: ⚡ Quick winAdd a rerender test for
projectprop changes.Please add a case that rerenders the hook with a different
projectand assertslocalis reinitialized. This will lock in the expected React hook lifecycle behavior and prevent stale-form regressions.🧪 Suggested test shape
+ it("project が切り替わったら local が再初期化される", () => { + const p1: CareerProjectForm = { ...sampleProject, name: "A" }; + const p2: CareerProjectForm = { ...sampleProject, name: "B" }; + const { result, rerender } = renderHook( + ({ project }) => useProjectModalForm(project), + { initialProps: { project: p1 } }, + ); + + act(() => result.current.updateField("name", "edited")); + expect(result.current.local.name).toBe("edited"); + + rerender({ project: p2 }); + expect(result.current.local.name).toBe("B"); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useProjectModalForm.test.ts` around lines 22 - 108, Add a test that verifies useProjectModalForm reinitializes when its project prop changes: render the hook with sampleProject, mutate result.current.local, then call the renderHook rerender with a different project (e.g., null or a different sample object) and assert that result.current.local matches the new project's initial shape (for null: empty defaults; for a new project: its fields, e.g., name, is_current, technology_stacks). Use the same renderHook/rerender pattern and reference result.current from useProjectModalForm to ensure the hook resets rather than preserving mutated state.frontend/src/hooks/useAuthSession.test.ts (1)
48-83: ⚡ Quick winAdd a navigation case for public → protected routing.
These tests only cover the initial pathname. The
authLoadingregression shows up when the hook mounts on a public route and later moves to a protected one, so a navigation-based case would lock that behavior down.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useAuthSession.test.ts` around lines 48 - 83, Add a new test that simulates navigating from a public route to a protected route to cover the regression: render the hook with renderHook(() => useAuthSession(), { wrapper: makeWrapper("/login") }), assert initial state (authLoading false, api.getCurrentUser not called), then simulate navigation to a protected path (e.g. update wrapper history to "/career" or call history.push inside act), mock api.getCurrentUser to resolve a user, waitFor the hook to update and assert api.getCurrentUser was called and result.current.user and sessionStorage were populated (use the same symbols useAuthSession, makeWrapper, api.getCurrentUser, waitFor, sessionStorage used in the existing tests).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tests/services/tasks/test_handlers_failure.py`:
- Around line 72-77: The test test_missing_user_id_raises_non_retryable is
removing multiple required keys from the Career payload which can mask
regressions; update this test (and the sibling test covering lines 78-84) to
construct a valid payload containing all required keys except user_id, then call
CareerAnalysisHandler.run with that payload to assert NonRetryableError,
ensuring only the targeted missing-key condition is tested; locate the payload
construction in test_missing_user_id_raises_non_retryable and in the paired test
and modify them to keep all other keys present while omitting only the specific
key under test.
In `@frontend/src/hooks/useAuthSession.ts`:
- Around line 98-103: The handleLogout function currently awaits logout() and if
that call rejects the local cleanup (sessionStorage.removeItem, setting
justLoggedOut.current and setUser(null)) never runs; change handleLogout to call
logout() inside a try/catch and perform the client-side cleanup in a finally
block so client state is cleared deterministically, and in the catch either log
the server error or surface it in a controlled way rather than letting it become
an unhandled rejection (keep references to logout(),
sessionStorage.removeItem("auth_user"), justLoggedOut.current, and setUser(null)
to locate the changes).
- Around line 54-61: The effect in useAuthSession currently clears the entire
search string when a github_error is present, losing other query params; update
the logic inside the useEffect to parse location.search with URLSearchParams,
remove only the "github_error" entry (params.delete("github_error")), build the
new search string from params.toString() and call navigate with the original
pathname plus the preserved search (omit the ?, or use empty search when no
params remain) while keeping replace: true, then call setGithubError(error);
reference useEffect, location, URLSearchParams, navigate, and setGithubError to
locate and change the code.
- Around line 63-96: The effect should set authLoading true before starting
restoration so route guards wait during public→protected transitions; inside the
useEffect (the block that checks user, PUBLIC_PATHS, justLoggedOut and then runs
the async IIFE calling getCurrentUser), call setAuthLoading(true) immediately
before invoking the async restoration (and keep the existing finally that calls
setAuthLoading(false)), ensuring you still skip this when the early-return
branch runs; update the logic around
useEffect/user/location.pathname/getCurrentUser/setAuthLoading to reflect this
change.
In `@frontend/src/hooks/useBlogAccountManager.ts`:
- Around line 46-55: The setAction setter can clobber a newer action when an
earlier async operation's finally calls setAction(platform, null); modify
setAction in useBlogAccountManager to accept an optional expectedAction
parameter (e.g. setAction(platform: PlatformKey, action: PlatformAction | null,
expectedAction?: PlatformAction)) and when action === null only delete the key
if prev[platform] === expectedAction (i.e. the action that scheduled the clear),
otherwise leave it unchanged; update all callers that clear the action (the
finally blocks referenced at lines 142/164/184/214) to pass the exact
PlatformAction they started as expectedAction so a later-started action won't be
cleared by an earlier completion.
In `@frontend/src/utils/taskStatus.ts`:
- Around line 10-15: The TaskStatus union type is missing the "failed" variant
which exists in backend/task flows; update the TaskStatus type definition
(symbol: TaskStatus in frontend/src/utils/taskStatus.ts) to include "failed"
alongside "pending", "processing", "retrying", "completed", and "dead_letter" so
all consumers and type-narrowing remain sound; after adding "failed", run
typechecks and update any switch/if exhaustiveness handling around TaskStatus to
include the new case.
---
Nitpick comments:
In `@frontend/src/hooks/useAuthSession.test.ts`:
- Around line 48-83: Add a new test that simulates navigating from a public
route to a protected route to cover the regression: render the hook with
renderHook(() => useAuthSession(), { wrapper: makeWrapper("/login") }), assert
initial state (authLoading false, api.getCurrentUser not called), then simulate
navigation to a protected path (e.g. update wrapper history to "/career" or call
history.push inside act), mock api.getCurrentUser to resolve a user, waitFor the
hook to update and assert api.getCurrentUser was called and result.current.user
and sessionStorage were populated (use the same symbols useAuthSession,
makeWrapper, api.getCurrentUser, waitFor, sessionStorage used in the existing
tests).
In `@frontend/src/hooks/useProjectModalForm.test.ts`:
- Around line 22-108: Add a test that verifies useProjectModalForm reinitializes
when its project prop changes: render the hook with sampleProject, mutate
result.current.local, then call the renderHook rerender with a different project
(e.g., null or a different sample object) and assert that result.current.local
matches the new project's initial shape (for null: empty defaults; for a new
project: its fields, e.g., name, is_current, technology_stacks). Use the same
renderHook/rerender pattern and reference result.current from
useProjectModalForm to ensure the hook resets rather than preserving mutated
state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c6445478-3641-4337-9652-825b37744480
📒 Files selected for processing (19)
backend/app/services/intelligence/github_analysis_service.pybackend/app/services/tasks/handlers/career_analysis.pybackend/tests/services/tasks/test_handlers_failure.pybackend/tests/test_worker_extended.pyfrontend/src/App.tsxfrontend/src/api/client.tsfrontend/src/components/analysis/TechBar.test.tsxfrontend/src/components/forms/ProjectModal.tsxfrontend/src/hooks/analysis/useAsyncAnalysisPage.test.tsfrontend/src/hooks/analysis/useAsyncAnalysisPage.tsfrontend/src/hooks/useAuthSession.test.tsfrontend/src/hooks/useAuthSession.tsfrontend/src/hooks/useBlogAccountManager.tsfrontend/src/hooks/useBlogSummaryPolling.tsfrontend/src/hooks/useCareerAnalysisPage.tsfrontend/src/hooks/useDocumentForm.test.tsfrontend/src/hooks/useProjectModalForm.test.tsfrontend/src/hooks/useProjectModalForm.tsfrontend/src/utils/taskStatus.ts
💤 Files with no reviewable changes (1)
- frontend/src/components/analysis/TechBar.test.tsx
Comment on lines
+10
to
+15
| export type TaskStatus = | ||
| | "pending" | ||
| | "processing" | ||
| | "retrying" | ||
| | "completed" | ||
| | "dead_letter"; |
There was a problem hiding this comment.
TaskStatus union is missing failed status.
Line 10-15 defines a backend contract type, but it omits failed, which is already treated as a meaningful task state elsewhere (e.g., retry flow). This can cause type drift and unsafe narrowing over time.
Suggested fix
export type TaskStatus =
| "pending"
| "processing"
| "retrying"
+ | "failed"
| "completed"
| "dead_letter";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/utils/taskStatus.ts` around lines 10 - 15, The TaskStatus union
type is missing the "failed" variant which exists in backend/task flows; update
the TaskStatus type definition (symbol: TaskStatus in
frontend/src/utils/taskStatus.ts) to include "failed" alongside "pending",
"processing", "retrying", "completed", and "dead_letter" so all consumers and
type-narrowing remain sound; after adding "failed", run typechecks and update
any switch/if exhaustiveness handling around TaskStatus to include the new case.
Owner
Author
Cross-Realm Refactor PR Report
Summary3 つの SSoT 違反を解消し、領域横断の同期忘れ事故が起きにくい構造に統合。
SSoT ConsolidationsHigh
Medium
Documentation Consolidations
正本ファイル（ Skipped
Validation
Build 失敗 → 修正の経緯初回 build 時に これは XR_refacter で意図した「FE 側でキー漏れを型エラーで検出する」効果が 実装段階で発動した好例（既存コードが string index で逃げていた箇所が明示的に処理されるようになった）。 Follow-ups
|
Owner
Author
Infra Refactor PR Report (Skipped 全件解消 + 設計レビュー)
Summary前回 PR の Skipped を全件解消した。HCL 重複率は 17.93% → 0% に劇的改善（全体: 3.10% → 2.24%、74 → 66 clones）。stg / prod を devforge_stack 化し、 Applied ChangesPhase A: stg / prod の devforge_stack 化
Phase B: variables.tf / moved.tf / outputs.tf の shared/ + symlink 共通化
Phase C: moved.tf の拡充
Phase D: cloud_tasks の queue_name derive 化（M2）
Phase E: monitoring 分割（M4）
sub module 化ではなく 同一 module 内のファイル分割 で済ませている（CLAUDE.md の「過剰な抽象化を避ける」方針に整合）。 Phase F: cloudflare 設定の tfvars 外出し
Phase G: devforge_stack から
|
| コマンド | 結果 |
|---|---|
make infra-fmt-check |
✅ pass（差分なし） |
make infra-validate-dev |
✅ Success! The configuration is valid. |
make infra-validate-stg |
✅ Success! The configuration is valid. |
make infra-validate-prod |
✅ Success! The configuration is valid. |
make dupe-check |
✅ 66 clones / 2.24% （前回 74 / 3.10%） |
dupe-check の改善内訳
| 言語 | 前回 (dev のみ devforge_stack 化) | 今回 (stg/prod も + symlink) | 改善 |
|---|---|---|---|
| HCL | 17.93% (8 clones) | 0% (0 clones) | -17.93pt |
| TypeScript | 4.86% | 4.86% | ─ |
| TSX | 0.62% | 0.62% | ─ |
| Python | 2.49% | 2.49% | ─ |
| Markdown | 0.59% | 0.59% | ─ |
| 全体 | 3.10% | 2.24% | -0.86pt |
設計レビュー（codex 実装の RV）
良い設計（特筆事項）
shared/+ symlink パターン: Terraform root module の variable / output / moved を物理的に 1 ファイルに集約しつつ、各 env を root module として残せる解。OpenTofu はファイルを読むだけなので validate も通るmoved.tfの symlink 共通化: 3 環境すべてが同じ移行履歴を辿るため正当。state 残存リスクへの保険- monitoring のファイル分割: sub module 化ではなくファイル分割で済ませている（「変更理由が複数だから分割」を遵守、過剰な抽象化を避けた）
cloud_tasks.queue_nameの module 内 derive:app_name+environmentを渡せばリテラル組み立てが不要になり、devforge_stack 側がさらにシンプルに- cloudflare 設定の tfvars 外出し: dev / stg / prod の差分が tfvars に集約され、main.tf からリテラルが完全に消えた
.jscpd.json の計測値について確認
ignore に以下 9 件を追加して symlink ファイルを除外している:
"**/infra/environments/dev/variables.tf",
"**/infra/environments/stg/variables.tf",
"**/infra/environments/prod/variables.tf",
"**/infra/environments/dev/moved.tf",
"**/infra/environments/stg/moved.tf",
"**/infra/environments/prod/moved.tf",
"**/infra/environments/dev/outputs.tf",
"**/infra/environments/stg/outputs.tf",
"**/infra/environments/prod/outputs.tf"判定: 妥当。symlink 経由で同一ファイルを 3 回スキャンすると jscpd が 100% 重複として誤検出するため、明示除外が正解。shared/ の正本だけがスキャン対象になり、HCL の真の重複だけが計測される。
注意点: HCL 0% は「真の重複ゼロ」ではなく「symlink 除外後の重複ゼロ」。docs/development.md か .claude/rules/common/duplication.md に「symlink 採用に伴う計測の前提」を 1 行追記すると親切（次回 PR で対応推奨）。
改善余地（コスメティック）: **/ prefix は jscpd の相対パス解釈では不要。infra/environments/dev/variables.tf だけで動く。動作には影響なし
軽微な懸念
| 観点 | 内容 | 対応 |
|---|---|---|
| Windows 開発者 | symlink は Windows の git では設定が必要（core.symlinks=true） |
当該開発者がいない or Nix devshell 内なら問題なし |
.terraform.lock.hcl の更新 |
stg / prod で hashicorp/google 7.22 → 7.23 に自動更新 |
validate 副作用、3 環境同一バージョンに揃った状態で意図通り |
template_version の取り扱い |
devforge_stack からは削除、outputs.tf 内のみで使用 | 設計判断としては妥当だが、将来 stack 側で必要になれば再追加 |
残課題（次 PR 候補）
docs/development.mdか.claude/rules/common/duplication.mdに symlink + jscpd ignore の説明追記: HCL 0% の誤読を防ぐ- L3 (cloudflare.production_branch 命名不一致): prod だけ env 名と branch 名が違う（"main"）件は既に tfvars に外出しされ、規約の議論ポイントだけ残る。ADR で記録するなら別 PR
.terraform.lock.hclのコミット方針: prod / stg は git 管理されている。lock の自動更新時の運用ルール明文化（PR 内で常に最新化、or 意図的更新時のみ）
Skipped → 解消状況
| 前回 PR の Skipped 項目 | 本 PR での対応 |
|---|---|
| H2 (variables.tf 95% 重複) | ✅ shared/variables.tf + symlink で物理統合 |
| M1 (moved.tf 重複) | ✅ shared/moved.tf + symlink で物理統合、firebase / resume_stack / google_project_service 旧 address からの移行ブロックも完備 |
| M2 (cloud_tasks.queue_name) | ✅ module 内で ${app_name}-ai-tasks-${env} で derive、devforge_stack からリテラル削除 |
| M4 (monitoring/main.tf 248 行) | ✅ 5 ファイル分割（notification_channels / uptime / auth_failures / rate_limits / task_failures） |
| L1 (output 5 個の重複) | ✅ shared/outputs.tf + symlink で物理統合 |
| L3 (cloudflare.production_branch 命名不一致) | ✅ tfvars に外出し、prod のみ "main" は明示的に tfvars に記録 |
| stg / prod 環境の devforge_stack 適用 | ✅ 完了 |
Follow-ups
- 次 PR:
docs/development.mdの更新（symlink + jscpd 計測の前提を明記） - 監視 alert の追加時は
monitoring/<新規責務>.tfを新設する運用ルール - variables.tf のさらなる整理（env で実質使われていない variable があれば削除）
- Windows 開発者が将来参画する場合の symlink ハンドリング（
.gitattributesで* text=auto eol=lf等の設定）
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/core/encryption.py (1)
9-11: ⚡ Quick winConsider using the constant in the error message for consistency.
Line 9 now correctly uses
env_keys.FIELD_ENCRYPTION_KEY, but the error message on Line 11 still uses the literal string. For complete centralization, consider:♻️ Suggested refactor
def _get_fernet() -> Fernet: key = os.getenv(env_keys.FIELD_ENCRYPTION_KEY, "").strip() if not key: - raise RuntimeError("FIELD_ENCRYPTION_KEY is not configured") + raise RuntimeError(f"{env_keys.FIELD_ENCRYPTION_KEY} is not configured") return Fernet(key.encode())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/core/encryption.py` around lines 9 - 11, The RuntimeError raised when the FIELD_ENCRYPTION_KEY is missing uses a hard-coded string; update the error to reference the centralized constant env_keys.FIELD_ENCRYPTION_KEY instead so the message stays consistent with the env lookup (change the RuntimeError in the block that reads key = os.getenv(env_keys.FIELD_ENCRYPTION_KEY, "").strip() to include env_keys.FIELD_ENCRYPTION_KEY in the exception text).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/api/paths.ts`:
- Line 30: Change the PATHS types so careerAnalysis endpoints accept only number
and align aiResume snapshot types with actual usage: update careerAnalysis.byId,
careerAnalysis.status, and careerAnalysis.retry to take id: number (not number |
string), and change aiResume.snapshotById to take id: number; then update any
related signatures so functions in frontend/src/api/ai-resume.ts (getSnapshot,
updateSnapshot, finalizeSnapshot, deleteSnapshot, downloadPdf, downloadMarkdown)
and any callers use number types consistently to match backend int routes.
---
Nitpick comments:
In `@backend/app/core/encryption.py`:
- Around line 9-11: The RuntimeError raised when the FIELD_ENCRYPTION_KEY is
missing uses a hard-coded string; update the error to reference the centralized
constant env_keys.FIELD_ENCRYPTION_KEY instead so the message stays consistent
with the env lookup (change the RuntimeError in the block that reads key =
os.getenv(env_keys.FIELD_ENCRYPTION_KEY, "").strip() to include
env_keys.FIELD_ENCRYPTION_KEY in the exception text).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4b2f721-29f4-4197-ac37-94ddd20b2432
📒 Files selected for processing (81)
.claude/CLAUDE.md.claude/rules/backend/python.md.claude/rules/common/duplication.md.claude/rules/frontend/typescript.md.claude/rules/infra/opentofu.md.claude/skills/BE_apply/SKILL.md.claude/skills/BE_refacter/SKILL.md.claude/skills/FE_apply/SKILL.md.claude/skills/FE_refacter/SKILL.md.claude/skills/INFRA_apply/SKILL.md.claude/skills/INFRA_refacter/SKILL.md.claude/skills/XR_apply/SKILL.md.claude/skills/XR_refacter/SKILL.md.githooks/pre-push.github/workflows/ci.yml.gitignore.jscpd.jsonMakefilebackend/app/core/encryption.pybackend/app/core/env_keys.pybackend/app/core/errors.pybackend/app/core/redis_client.pybackend/app/core/settings.pybackend/app/main.pybackend/app/routers/internal.pybackend/app/services/tasks/cloud_tasks.pybackend/app/services/tasks/factory.pybackend/tests/services/tasks/test_handlers_failure.pyfrontend/src/api/ai-resume.tsfrontend/src/api/auth.tsfrontend/src/api/blog.tsfrontend/src/api/career-analysis.tsfrontend/src/api/client.tsfrontend/src/api/intelligence.tsfrontend/src/api/master-data.tsfrontend/src/api/notifications.tsfrontend/src/api/paths.tsfrontend/src/api/resumes.tsfrontend/src/components/ui/ErrorToast.tsxfrontend/src/constants/errorCodes.tsfrontend/src/constants/errorMessages.tsfrontend/src/hooks/useAuthSession.tsfrontend/src/hooks/useBlogAccountManager.tsinfra/environments/dev/.terraform.lock.hclinfra/environments/dev/main.tfinfra/environments/dev/moved.tfinfra/environments/dev/moved.tfinfra/environments/dev/outputs.tfinfra/environments/dev/terraform.tfvarsinfra/environments/dev/variables.tfinfra/environments/dev/variables.tfinfra/environments/prod/.terraform.lock.hclinfra/environments/prod/main.tfinfra/environments/prod/moved.tfinfra/environments/prod/moved.tfinfra/environments/prod/outputs.tfinfra/environments/prod/terraform.tfvarsinfra/environments/prod/variables.tfinfra/environments/prod/variables.tfinfra/environments/shared/moved.tfinfra/environments/shared/outputs.tfinfra/environments/shared/variables.tfinfra/environments/stg/.terraform.lock.hclinfra/environments/stg/main.tfinfra/environments/stg/moved.tfinfra/environments/stg/moved.tfinfra/environments/stg/outputs.tfinfra/environments/stg/terraform.tfvarsinfra/environments/stg/variables.tfinfra/environments/stg/variables.tfinfra/modules/cloud_tasks/main.tfinfra/modules/cloud_tasks/variables.tfinfra/modules/devforge_stack/main.tfinfra/modules/devforge_stack/outputs.tfinfra/modules/devforge_stack/variables.tfinfra/modules/monitoring/auth_failures.tfinfra/modules/monitoring/main.tfinfra/modules/monitoring/notification_channels.tfinfra/modules/monitoring/rate_limits.tfinfra/modules/monitoring/task_failures.tfinfra/modules/monitoring/uptime.tf
💤 Files with no reviewable changes (1)
- infra/modules/monitoring/main.tf
✅ Files skipped from review due to trivial changes (15)
- .gitignore
- .claude/rules/backend/python.md
- .claude/rules/frontend/typescript.md
- frontend/src/api/master-data.ts
- .jscpd.json
- infra/environments/prod/terraform.tfvars
- infra/environments/dev/.terraform.lock.hcl
- .claude/rules/common/duplication.md
- .claude/rules/infra/opentofu.md
- backend/app/main.py
- backend/app/core/errors.py
- infra/environments/stg/.terraform.lock.hcl
- infra/environments/prod/moved.tf
- .claude/CLAUDE.md
- .claude/skills/BE_apply/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/api/client.ts
- frontend/src/hooks/useBlogAccountManager.ts
- frontend/src/hooks/useAuthSession.ts
Owner
Author
Summary
Applied ChangesHigh
Medium
Low
Test ChangesRemoved (= 別ファイルへの分割移動 / 一切のテスト削除なし)
Added
Duplication Resolved
残した偶発的重複 (抽出見送り)
Structure Changes旧ファイル ( Skipped採用しなかった指摘と理由:
Validation
Follow-ups
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
backend/tests/auth/test_token_manager.py (1)
57-58: ⚡ Quick winNarrow the expected exception type in the JWT mismatch test.
Line 57 catches
Exception, which can mask unrelated failures. Assert the jose-specific exception instead. In python-jose 3.5.0,jwt.decoderaisesJWTErrorwhen the algorithm in the token header does not match the allowed algorithms.Proposed patch
from jose import jwt +from jose.exceptions import JWTError with pytest.raises(Exception): + with pytest.raises(JWTError): jwt.decode(hs256_token, _test_public_key, algorithms=["RS256"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/auth/test_token_manager.py` around lines 57 - 58, The test currently uses a broad pytest.raises(Exception) around jwt.decode(hs256_token, _test_public_key, algorithms=["RS256"]); narrow this to the jose-specific exception by importing JWTError (e.g. from jose.exceptions import JWTError) and changing the assertion to pytest.raises(JWTError) so the test specifically expects the algorithm-mismatch error when decoding hs256_token with algorithms=["RS256"].backend/app/routers/blog/sync.py (1)
49-54: ⚡ Quick winLog unexpected sync failures before mapping to 502.
The generic fallback currently drops operational context. Add
logger.exception(...)before raising the 502 to keep stack traces for incident triage.Proposed patch
except Exception as exc: - # UnsupportedBlogPlatformError は上の except で先に捕捉される + logger.exception("Unexpected blog sync failure: account_id=%s user_id=%s", account_id, user.id) raise HTTPException( status_code=502, detail=get_error("blog.sync_failed"), ) from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/routers/blog/sync.py` around lines 49 - 54, In the generic except block that currently raises HTTPException(status_code=502, detail=get_error("blog.sync_failed")) (the except Exception as exc handler), log the unexpected exception first by calling logger.exception(...) so the stack trace and context are preserved before mapping to the 502; ensure you reference the caught variable exc (e.g., logger.exception("Unexpected blog sync failure", exc_info=exc) or equivalent) and then raise the HTTPException as before—note that UnsupportedBlogPlatformError is still handled by the earlier except.backend/tests/auth/test_endpoints.py (1)
235-249: ⚡ Quick winAlways reset limiter in
finallyto avoid cross-test leakage on early failures.If an assertion fails before the trailing reset, limiter state can contaminate later tests.
Proposed refactor
def test_auth_me_rate_limited_after_threshold(client) -> None: """/auth/me が 60/分の上限を超えると 429 を返すことを確認する。""" auth_header(client, "rl-user") limiter.reset() - statuses: list[int] = [] - for _i in range(65): - resp = client.get("/auth/me") - statuses.append(resp.status_code) - if resp.status_code == 429: - break - assert 429 in statuses - # 上限到達前に少なくとも数件は 200 を返している - assert statuses.count(200) >= 50 - limiter.reset() + try: + statuses: list[int] = [] + for _i in range(65): + resp = client.get("/auth/me") + statuses.append(resp.status_code) + if resp.status_code == 429: + break + assert 429 in statuses + # 上限到達前に少なくとも数件は 200 を返している + assert statuses.count(200) >= 50 + finally: + limiter.reset()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/auth/test_endpoints.py` around lines 235 - 249, The test test_auth_me_rate_limited_after_threshold currently calls limiter.reset() only at the start and end, which can leak limiter state if an assertion fails; wrap the test request loop and assertions in a try/finally and call limiter.reset() in the finally block (or convert to a pytest fixture teardown) so limiter.reset() is always executed; locate the limiter.reset() calls in test_auth_me_rate_limited_after_threshold and ensure the trailing reset is moved inside a finally to guarantee cleanup even on early failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tests/auth/test_endpoints.py`:
- Around line 191-194: The helper _auth_failed_records currently accesses
LogRecord.message which doesn't exist and will raise AttributeError; update the
filter to call r.getMessage() instead (i.e., in _auth_failed_records replace
r.message == "auth_failed" with r.getMessage() == "auth_failed") while keeping
the logger name check (r.name == "devforge") so only devforge WARNING
auth_failed events are selected.
In `@backend/tests/auth/test_oauth_flow.py`:
- Around line 176-192: The test
test_github_login_url_uses_callback_base_url_when_set currently sets
CALLBACK_BASE_URL without a scheme which yields an invalid OAuth redirect_uri;
update the patched env in that test to include the https:// scheme (e.g.
"https://devforge-dev-XXXXX-an.a.run.app") and change the assertion for
redirect_uri to expect the full URL with scheme and path (e.g.
"https://devforge-dev-XXXXX-an.a.run.app/github/callback") so the generated
authorization_url uses a valid redirect URI.
In `@backend/tests/test_worker/test_execute_task.py`:
- Around line 118-124: The test isn't actually simulating a rollback failure;
replace the no-op and manual rollback with a real DB error before calling
_safe_rollback: arrange the db_session mock so that a DB operation raises (e.g.,
set db_session.execute.side_effect or db_session.rollback.side_effect to an
Exception) to put the session into a failing state, then call
_safe_rollback(db_session) and assert that no exception escapes. Target the
existing symbols _safe_rollback and db_session (e.g., adjust db_session.execute
or db_session.rollback side_effect) so the test validates the failure-recovery
path.
---
Nitpick comments:
In `@backend/app/routers/blog/sync.py`:
- Around line 49-54: In the generic except block that currently raises
HTTPException(status_code=502, detail=get_error("blog.sync_failed")) (the except
Exception as exc handler), log the unexpected exception first by calling
logger.exception(...) so the stack trace and context are preserved before
mapping to the 502; ensure you reference the caught variable exc (e.g.,
logger.exception("Unexpected blog sync failure", exc_info=exc) or equivalent)
and then raise the HTTPException as before—note that
UnsupportedBlogPlatformError is still handled by the earlier except.
In `@backend/tests/auth/test_endpoints.py`:
- Around line 235-249: The test test_auth_me_rate_limited_after_threshold
currently calls limiter.reset() only at the start and end, which can leak
limiter state if an assertion fails; wrap the test request loop and assertions
in a try/finally and call limiter.reset() in the finally block (or convert to a
pytest fixture teardown) so limiter.reset() is always executed; locate the
limiter.reset() calls in test_auth_me_rate_limited_after_threshold and ensure
the trailing reset is moved inside a finally to guarantee cleanup even on early
failures.
In `@backend/tests/auth/test_token_manager.py`:
- Around line 57-58: The test currently uses a broad pytest.raises(Exception)
around jwt.decode(hs256_token, _test_public_key, algorithms=["RS256"]); narrow
this to the jose-specific exception by importing JWTError (e.g. from
jose.exceptions import JWTError) and changing the assertion to
pytest.raises(JWTError) so the test specifically expects the algorithm-mismatch
error when decoding hs256_token with algorithms=["RS256"].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 76055f7c-2e91-40f1-870c-b5eeff67fbc1
📒 Files selected for processing (47)
backend/app/routers/auth/endpoints.pybackend/app/routers/auth/token_manager.pybackend/app/routers/blog.pybackend/app/routers/blog/__init__.pybackend/app/routers/blog/accounts.pybackend/app/routers/blog/score.pybackend/app/routers/blog/summarize.pybackend/app/routers/blog/sync.pybackend/app/routers/career_analysis.pybackend/app/routers/intelligence.pybackend/app/routers/resumes.pybackend/app/schemas/__init__.pybackend/app/schemas/career_analysis.pybackend/app/schemas/shared.pybackend/app/services/markdown/generators/resume_generator.pybackend/app/services/pdf/generators/resume_generator.pybackend/app/services/shared/resume_format.pybackend/tests/auth/__init__.pybackend/tests/auth/test_endpoints.pybackend/tests/auth/test_oauth_flow.pybackend/tests/auth/test_token_manager.pybackend/tests/blog/__init__.pybackend/tests/blog/test_accounts.pybackend/tests/blog/test_score.pybackend/tests/blog/test_summarize.pybackend/tests/blog/test_sync.pybackend/tests/security/__init__.pybackend/tests/security/_helpers.pybackend/tests/security/test_admin_authorization.pybackend/tests/security/test_boundary_values.pybackend/tests/security/test_idor.pybackend/tests/security/test_no_auth.pybackend/tests/security/test_sql_injection.pybackend/tests/services/tasks/test_handlers_success.pybackend/tests/test_auth.pybackend/tests/test_career_analysis_api.pybackend/tests/test_oauth_flow.pybackend/tests/test_retry_flow.pybackend/tests/test_security_edges.pybackend/tests/test_worker/__init__.pybackend/tests/test_worker/_helpers.pybackend/tests/test_worker/test_blog_summarize.pybackend/tests/test_worker/test_career_analysis.pybackend/tests/test_worker/test_execute_task.pybackend/tests/test_worker/test_github_analysis.pybackend/tests/test_worker_extended.pyfrontend/src/api/paths.ts
💤 Files with no reviewable changes (5)
- backend/app/routers/blog.py
- backend/tests/test_oauth_flow.py
- backend/tests/test_auth.py
- backend/tests/test_worker_extended.py
- backend/tests/test_security_edges.py
✅ Files skipped from review due to trivial changes (4)
- backend/app/routers/blog/init.py
- backend/app/routers/career_analysis.py
- backend/app/routers/intelligence.py
- backend/app/schemas/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/api/paths.ts
Comment on lines
+191
to
+194
| def _auth_failed_records(records: list[logging.LogRecord]) -> list[logging.LogRecord]: | ||
| """auth_failed イベント (devforge ロガー WARNING) のみ抽出する。""" | ||
| return [r for r in records if r.name == "devforge" and r.message == "auth_failed"] | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import logging
r = logging.LogRecord("devforge", logging.WARNING, __file__, 1, "auth_failed", (), None)
print("has message attr before formatting:", hasattr(r, "message"))
print("safe payload via getMessage():", r.getMessage())
PYRepository: yusuke0610/devforge
Length of output: 148
🏁 Script executed:
# First, let's check the actual implementation in the test file
cat -n backend/tests/auth/test_endpoints.py | sed -n '185,200p'Repository: yusuke0610/devforge
Length of output: 787
🏁 Script executed:
# Also check if there are any uses of _auth_failed_records in the file
rg "_auth_failed_records" backend/tests/auth/test_endpoints.py -B 2 -A 2Repository: yusuke0610/devforge
Length of output: 824
🏁 Script executed:
# Check the full context of how logging is set up in this test
rg -B 5 -A 5 "caplog\|LogRecord" backend/tests/auth/test_endpoints.py | head -50Repository: yusuke0610/devforge
Length of output: 45
🏁 Script executed:
# Check if there's a pytest configuration or logging setup in the codebase
fd -e "conftest.py" -e "pyproject.toml" -e "pytest.ini" backend/tests/Repository: yusuke0610/devforge
Length of output: 425
🏁 Script executed:
# Look for any logging configuration that might affect LogRecord attributes
rg -i "logging.config\|logging.format\|LogRecord" backend/tests/ --type py | head -30Repository: yusuke0610/devforge
Length of output: 45
🏁 Script executed:
# Let's check the actual test execution to understand caplog behavior
python - <<'PY'
import logging
import pytest
# Simulate what pytest's caplog does
class MockCaplogRecords:
def __init__(self):
self.records = []
self.handler = logging.Handler()
self.handler.emit = lambda record: self.records.append(record)
def capture(self):
logger = logging.getLogger("devforge")
logger.addHandler(self.handler)
logger.warning("auth_failed")
logger.removeHandler(self.handler)
return self.records
caplog_mock = MockCaplogRecords()
records = caplog_mock.capture()
if records:
r = records[0]
print(f"LogRecord has 'message' attribute: {hasattr(r, 'message')}")
print(f"LogRecord has 'msg' attribute: {hasattr(r, 'msg')}")
print(f"LogRecord.msg value: {r.msg}")
print(f"LogRecord.getMessage() result: {r.getMessage()}")
print(f"Dir of record: {[attr for attr in dir(r) if 'msg' in attr.lower() or 'message' in attr.lower()]}")
PYRepository: yusuke0610/devforge
Length of output: 253
Use LogRecord.getMessage() instead of record.message.
The message attribute does not exist on LogRecord objects. Accessing r.message will raise AttributeError at runtime. Use r.getMessage() to retrieve the formatted message safely.
Proposed fix
def _auth_failed_records(records: list[logging.LogRecord]) -> list[logging.LogRecord]:
"""auth_failed イベント (devforge ロガー WARNING) のみ抽出する。"""
- return [r for r in records if r.name == "devforge" and r.message == "auth_failed"]
+ return [r for r in records if r.name == "devforge" and r.getMessage() == "auth_failed"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _auth_failed_records(records: list[logging.LogRecord]) -> list[logging.LogRecord]: | |
| """auth_failed イベント (devforge ロガー WARNING) のみ抽出する。""" | |
| return [r for r in records if r.name == "devforge" and r.message == "auth_failed"] | |
| def _auth_failed_records(records: list[logging.LogRecord]) -> list[logging.LogRecord]: | |
| """auth_failed イベント (devforge ロガー WARNING) のみ抽出する。""" | |
| return [r for r in records if r.name == "devforge" and r.getMessage() == "auth_failed"] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/auth/test_endpoints.py` around lines 191 - 194, The helper
_auth_failed_records currently accesses LogRecord.message which doesn't exist
and will raise AttributeError; update the filter to call r.getMessage() instead
(i.e., in _auth_failed_records replace r.message == "auth_failed" with
r.getMessage() == "auth_failed") while keeping the logger name check (r.name ==
"devforge") so only devforge WARNING auth_failed events are selected.
Comment on lines
+176
to
+192
| def test_github_login_url_uses_callback_base_url_when_set(client) -> None: | ||
| """CALLBACK_BASE_URL が設定されている場合、x-forwarded-host より優先されることを確認する。""" | ||
| with patch.dict(os.environ, {"CALLBACK_BASE_URL": "devforge-dev-XXXXX-an.a.run.app"}): | ||
| response = client.get( | ||
| "/auth/github/login-url", | ||
| headers={ | ||
| "Origin": "http://localhost:8788", | ||
| "Host": "devforge-dev-XXXXX-an.a.run.app", | ||
| "X-Forwarded-Proto": "https", | ||
| }, | ||
| ) | ||
|
|
||
| assert response.status_code == 200 | ||
| parsed = urlparse(response.json()["authorization_url"]) | ||
| redirect_uri = parse_qs(parsed.query)["redirect_uri"][0] | ||
| assert redirect_uri == "devforge-dev-XXXXX-an.a.run.app/github/callback" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does GitHub OAuth require redirect_urito be an absolute URL including scheme (e.g.,https://...)?
💡 Result:
Yes, the redirect_uri used in GitHub OAuth flows must be an absolute URL that includes the scheme (e.g., https://) [1][2]. According to official GitHub documentation, when you provide a redirect_uri parameter during the authorization request, it is validated against the callback URL configured in your OAuth app settings [1][2]. Specifically, the host (excluding sub-domains) and port of the provided redirect_uri must exactly match the callback URL configured in your app settings, and the path must reference a subdirectory of that callback URL [1][2]. For native applications, GitHub also supports loopback URLs (e.g., http://127.0.0.1:1234/path), which must also be absolute URLs including the scheme and port [1][2]. The OAuth RFC, which GitHub follows, recommends using loopback literals like 127.0.0.1 or IPv6::1 rather than localhost [1][2].
Citations:
- 1: https://docs.github.com/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps
- 2: https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps
🏁 Script executed:
# First, find the test file and understand its context
fd -t f "test_oauth_flow.py" backend/Repository: yusuke0610/devforge
Length of output: 101
🏁 Script executed:
# Search for the /auth/github/login-url endpoint
rg -n "github/login-url" --type py -A 10 -B 2Repository: yusuke0610/devforge
Length of output: 5803
🏁 Script executed:
# Search for how CALLBACK_BASE_URL is used in the codebase
rg -n "CALLBACK_BASE_URL" --type py -A 3 -B 1Repository: yusuke0610/devforge
Length of output: 2687
🏁 Script executed:
# Find the build_oauth_flow_and_redirect function implementation
rg -n "def build_oauth_flow_and_redirect" --type py -A 30Repository: yusuke0610/devforge
Length of output: 45
🏁 Script executed:
# Also check how it constructs the redirect_uri
rg -n "redirect_uri" backend/app/routers/auth/oauth_flow.py -B 2 -A 2Repository: yusuke0610/devforge
Length of output: 1094
🏁 Script executed:
# Find the get_frontend_origin function
rg -n "def get_frontend_origin" --type py -A 10Repository: yusuke0610/devforge
Length of output: 844
🏁 Script executed:
# Also find get_callback_base_url to understand its behavior
rg -n "def get_callback_base_url" --type py -A 5Repository: yusuke0610/devforge
Length of output: 585
Add https:// scheme to CALLBACK_BASE_URL in test and assertion.
GitHub OAuth rejects redirect_uri values without a scheme (i.e., https://example.com/path not example.com/path). This test currently allows a scheme-less configuration, which produces an invalid OAuth redirect URI and masks real authentication failures if misconfigured in production.
Proposed patch
- with patch.dict(os.environ, {"CALLBACK_BASE_URL": "devforge-dev-XXXXX-an.a.run.app"}):
+ with patch.dict(os.environ, {"CALLBACK_BASE_URL": "https://devforge-dev-XXXXX-an.a.run.app"}):
@@
- assert redirect_uri == "devforge-dev-XXXXX-an.a.run.app/github/callback"
+ assert redirect_uri == "https://devforge-dev-XXXXX-an.a.run.app/github/callback"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_github_login_url_uses_callback_base_url_when_set(client) -> None: | |
| """CALLBACK_BASE_URL が設定されている場合、x-forwarded-host より優先されることを確認する。""" | |
| with patch.dict(os.environ, {"CALLBACK_BASE_URL": "devforge-dev-XXXXX-an.a.run.app"}): | |
| response = client.get( | |
| "/auth/github/login-url", | |
| headers={ | |
| "Origin": "http://localhost:8788", | |
| "Host": "devforge-dev-XXXXX-an.a.run.app", | |
| "X-Forwarded-Proto": "https", | |
| }, | |
| ) | |
| assert response.status_code == 200 | |
| parsed = urlparse(response.json()["authorization_url"]) | |
| redirect_uri = parse_qs(parsed.query)["redirect_uri"][0] | |
| assert redirect_uri == "devforge-dev-XXXXX-an.a.run.app/github/callback" | |
| def test_github_login_url_uses_callback_base_url_when_set(client) -> None: | |
| """CALLBACK_BASE_URL が設定されている場合、x-forwarded-host より優先されることを確認する。""" | |
| with patch.dict(os.environ, {"CALLBACK_BASE_URL": "https://devforge-dev-XXXXX-an.a.run.app"}): | |
| response = client.get( | |
| "/auth/github/login-url", | |
| headers={ | |
| "Origin": "http://localhost:8788", | |
| "Host": "devforge-dev-XXXXX-an.a.run.app", | |
| "X-Forwarded-Proto": "https", | |
| }, | |
| ) | |
| assert response.status_code == 200 | |
| parsed = urlparse(response.json()["authorization_url"]) | |
| redirect_uri = parse_qs(parsed.query)["redirect_uri"][0] | |
| assert redirect_uri == "https://devforge-dev-XXXXX-an.a.run.app/github/callback" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/auth/test_oauth_flow.py` around lines 176 - 192, The test
test_github_login_url_uses_callback_base_url_when_set currently sets
CALLBACK_BASE_URL without a scheme which yields an invalid OAuth redirect_uri;
update the patched env in that test to include the https:// scheme (e.g.
"https://devforge-dev-XXXXX-an.a.run.app") and change the assertion for
redirect_uri to expect the full URL with scheme and path (e.g.
"https://devforge-dev-XXXXX-an.a.run.app/github/callback") so the generated
authorization_url uses a valid redirect URI.
Comment on lines
+118
to
+124
| # コミット失敗をシミュレートしてセッションを PendingRollback 状態にする | ||
| db_session.execute.__self__ if hasattr(db_session.execute, "__self__") else None | ||
| db_session.rollback() # まず手動でロールバックして dirty 状態を作る | ||
|
|
||
| # _safe_rollback は例外を上げないこと | ||
| _safe_rollback(db_session) | ||
|
|
There was a problem hiding this comment.
_safe_rollback の失敗系シミュレーションが実質行われていません。
Line 119 は no-op で、Line 120 は正常セッションへの rollback なので、「失敗後の復旧」検証になっていません。実際に DB エラーを発生させてから _safe_rollback を呼ぶ形にしてください。
🔧 Suggested test adjustment
from unittest.mock import AsyncMock, MagicMock, patch
import pytest
+from sqlalchemy import text
from app.models import BlogSummaryCache
...
- # コミット失敗をシミュレートしてセッションを PendingRollback 状態にする
- db_session.execute.__self__ if hasattr(db_session.execute, "__self__") else None
- db_session.rollback() # まず手動でロールバックして dirty 状態を作る
+ # DB エラーを発生させて失敗状態を作る
+ with pytest.raises(Exception):
+ db_session.execute(text("SELECT * FROM __missing_table__"))
# _safe_rollback は例外を上げないこと
_safe_rollback(db_session)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # コミット失敗をシミュレートしてセッションを PendingRollback 状態にする | |
| db_session.execute.__self__ if hasattr(db_session.execute, "__self__") else None | |
| db_session.rollback() # まず手動でロールバックして dirty 状態を作る | |
| # _safe_rollback は例外を上げないこと | |
| _safe_rollback(db_session) | |
| # DB エラーを発生させて失敗状態を作る | |
| with pytest.raises(Exception): | |
| db_session.execute(text("SELECT * FROM __missing_table__")) | |
| # _safe_rollback は例外を上げないこと | |
| _safe_rollback(db_session) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_worker/test_execute_task.py` around lines 118 - 124, The
test isn't actually simulating a rollback failure; replace the no-op and manual
rollback with a real DB error before calling _safe_rollback: arrange the
db_session mock so that a DB operation raises (e.g., set
db_session.execute.side_effect or db_session.rollback.side_effect to an
Exception) to put the session into a failing state, then call
_safe_rollback(db_session) and assert that no exception escapes. Target the
existing symbols _safe_rollback and db_session (e.g., adjust db_session.execute
or db_session.rollback side_effect) so the test validates the failure-recovery
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.
Backend Refactor: タスクハンドラ失敗パスを NonRetryableError に統一
Summary
return」と汎用RuntimeErrorをNonRetryableErrorraise に統一し、worker のdead_letter遷移を確実にする。Background
backend のリファクタリングレビューで、以下の不整合が見つかった:
career_analysis.py:38-40—analysisが None のときlogger.error+returnの silent return。worker は例外を受け取らないため正常終了として処理し、UI 側で「completed」として誤観測される。github_analysis_service.py:35,41—payload["user_id"]でKeyError、cache 不在時にRuntimeError。どちらも worker の汎用except Exception経路でリトライ対象になってしまうが、本質的にはディスパッチ側のバグであり、再試行しても回復しない。前回 turn で
blog_summarize.pyは同パターンを既にNonRetryableError化済みだったため、3 ハンドラ間で挙動を揃える必要があった。CLAUDE.md より（再掲）:
Changes
app/services/tasks/handlers/career_analysis.pyuser_id/record_id/target_position）欠落時にNonRetryableErrorを raise（missingリストをメッセージに含める）。NonRetryableErrorraise に置換。payload["..."]の直接アクセスを.get()に統一し、KeyErrorの発生経路を排除。app/services/intelligence/github_analysis_service.pypayload["user_id"]のKeyError→NonRetryableError。RuntimeError→NonRetryableError。backend/tests/services/tasks/test_handlers_failure.py（新規, 7 ケース）3 ハンドラの失敗パスを横断的に固定化:
TestBlogSummarizeHandlerFailures: user_id 欠落 / cache 不在TestCareerAnalysisHandlerFailures: user_id 欠落 / record_id 欠落 / record 不在TestGithubAnalysisHandlerFailures: user_id 欠落 / cache 不在すべて
pytest.raises(NonRetryableError)で明示的に検証（CLAUDE.md「失敗パスをpytest.raises(ExpectedError)で必ず assert する」原則）。backend/tests/test_worker_extended.py（旧契約 assert の更新）CLAUDE.md「契約変更時は既存テストの assert を必ず見直す。旧契約を固定化したテスト（例:
test_no_cache_returns_earlyのような silent-return アサーション）が残ると修正の意図が後退する。テスト名と本体の両方を更新する」原則に従い、以下を改名+書き換え:test_no_cache_raises_runtime_error→test_no_cache_raises_non_retryabletest_no_record_returns_early→test_no_record_raises_non_retryableTDD フロー
pytest tests/services/tasks/test_handlers_failure.pyで 5/7 失敗（career_analysis 3 / github_analysis 2）。blog_summarize2 件は前回修正済みで pass。make test-backend全件 pass。Validation
nix develop --command bash -c "cd backend && .venv/bin/python -m pytest tests/services/tasks/test_handlers_failure.py -v"→ 7/7 passTest plan
make test-backendで全 503 件 pass を確認make lint-backendcleandead_letter状態になることを確認（任意）failedステータス）が_create_notification経由でユーザーに届くことを stg で確認（任意）Impact / Risk
NonRetryableErrorは Cloud Tasks のリトライを止めるため、不正 payload で無限リトライしていたケースがあれば即停止する（Cloud Tasks のキュー負荷低減）。Out of scope
Backend リファクタリングレビューで挙げた以下は本 PR では未対応（別 PR で順次対応予定）:
routers/blog.pyの例外マッピング共通化worker.pyの_run_*テスト向けシム削除routers/blog.pyの package 化（accounts / articles / summary）services/blog/collector.pyのプラットフォーム別分割🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Infra