From fdda16442a123faa88269fde1a8bebccaed71e87 Mon Sep 17 00:00:00 2001 From: choiminyeong Date: Wed, 26 Aug 2026 12:54:46 +0900 Subject: [PATCH] Review and freeze TASK-004/005/006; fix 409/422 typed failures Independent read-only review of 8a47d30/bbbff77/16c46a5 per handoff/claude-master-handoff.md's review-and-freeze step. Verified build/test/lint independently rather than trusting recorded evidence. Fixed P2: requireSuccess() collapsed HTTP 409/422 into the generic InvalidResponse, which would have blocked TASK-007's planned conflict/unprocessable error states. Added ApiFailure.Conflict and ApiFailure.Unprocessable with test coverage. Recorded a P3 (CPU-bound work on the Main dispatcher in CodeBrowserViewModel) for TASK-016 rather than fixing it now. Closed TASK-005/006's mutual contract-freeze checkboxes. No P0/P1 found. Co-Authored-By: Claude Sonnet 5 --- .../ubgitea/core/network/HttpTransport.kt | 4 ++++ .../GiteaRepositoryBrowserApiTest.kt | 8 +++++++- docs/development-log.md | 15 +++++++++++++++ tasks/004-gitea-api-client.md | 11 ++++++++++- tasks/005-repository-branch-ui.md | 15 +++++++++++++-- tasks/006-code-browser.md | 18 +++++++++++++++--- 6 files changed, 64 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/com/ubisam/ubgitea/core/network/HttpTransport.kt b/app/src/main/java/com/ubisam/ubgitea/core/network/HttpTransport.kt index 9a0588e..7476969 100644 --- a/app/src/main/java/com/ubisam/ubgitea/core/network/HttpTransport.kt +++ b/app/src/main/java/com/ubisam/ubgitea/core/network/HttpTransport.kt @@ -81,6 +81,8 @@ sealed class ApiFailure(message: String) : Exception(message) { class AuthenticationExpired : ApiFailure("Authentication expired") class PermissionDenied : ApiFailure("Permission denied") class NotFound : ApiFailure("Resource not found") + class Conflict : ApiFailure("Conflict") + class Unprocessable : ApiFailure("Unprocessable request") class RateLimited : ApiFailure("Rate limited") class Network(cause: IOException) : ApiFailure("Network unavailable") { init { @@ -96,6 +98,8 @@ fun HttpResponse.requireSuccess(): HttpResponse = when (statusCode) { 401 -> throw ApiFailure.AuthenticationExpired() 403 -> throw ApiFailure.PermissionDenied() 404 -> throw ApiFailure.NotFound() + 409 -> throw ApiFailure.Conflict() + 422 -> throw ApiFailure.Unprocessable() 429 -> throw ApiFailure.RateLimited() in 500..599 -> throw ApiFailure.Server() else -> throw ApiFailure.InvalidResponse() diff --git a/app/src/test/java/com/ubisam/ubgitea/data/repositories/GiteaRepositoryBrowserApiTest.kt b/app/src/test/java/com/ubisam/ubgitea/data/repositories/GiteaRepositoryBrowserApiTest.kt index d6fbb5c..ace5dbe 100644 --- a/app/src/test/java/com/ubisam/ubgitea/data/repositories/GiteaRepositoryBrowserApiTest.kt +++ b/app/src/test/java/com/ubisam/ubgitea/data/repositories/GiteaRepositoryBrowserApiTest.kt @@ -140,10 +140,16 @@ class GiteaRepositoryBrowserApiTest { } @Test - fun `http status mapping distinguishes not found and rate limited`() { + fun `http status mapping distinguishes not found conflict unprocessable and rate limited`() { assertThrows(ApiFailure.NotFound::class.java) { HttpResponse(404, "", emptyMap()).requireSuccess() } + assertThrows(ApiFailure.Conflict::class.java) { + HttpResponse(409, "", emptyMap()).requireSuccess() + } + assertThrows(ApiFailure.Unprocessable::class.java) { + HttpResponse(422, "", emptyMap()).requireSuccess() + } assertThrows(ApiFailure.RateLimited::class.java) { HttpResponse(429, "", emptyMap()).requireSuccess() } diff --git a/docs/development-log.md b/docs/development-log.md index 4351910..5ec3de6 100644 --- a/docs/development-log.md +++ b/docs/development-log.md @@ -149,3 +149,18 @@ This is the append-only implementation and handoff record shared by Codex and Cl - Known limitations: Physical-device interaction has not yet been rerun. TASK-005's reviewed contract-freeze criterion remains open pending later independent review. - Next recommended task: TASK-009 immutable history/change/diff API contracts, followed by TASK-007 read-only history and change UI. - Next owner: Codex for the continuous serial stream. + +## 2026-08-26 — TASK-004/005/006 independent review and freeze + +- Agent and role: Claude, independent reviewer per `handoff/claude-master-handoff.md`'s "review and freeze" first action; human product owner authorized Claude as incoming primary implementer with handback to Codex kept possible at any time. +- Commit/diff: reviewed `8a47d30` (TASK-004), `bbbff77` (TASK-005), `16c46a5` (TASK-006) read-only, then fixed one finding on branch `claude/review-005-006-freeze` from `16c46a5`. +- Review scope and method: Independently reran `assembleDebug testDebugUnitTest lintDebug --rerun-tasks` (did not trust the recorded evidence alone) and read `GiteaRepositoryBrowserApi.kt`, `HttpTransport.kt`, `RepositoryListViewModel.kt`, `RepositoryReferencePresenter.kt`, `CodeBrowserViewModel.kt`, `CodeBrowserNavigation.kt`, and `MainActivity.kt` against the master handoff's checklist: races/cancellation, root back, rotation restoration, URL encoding, immutable snapshots, page overflow, and 404/409/422 mapping. TASK-009's commit was not yet available (still uncommitted upstream) and was excluded from this pass per human product owner direction. +- Findings: + - P2 (fixed): `core/network/HttpTransport.kt` `requireSuccess()` mapped 401/403/404/429/5xx but let 409 and 422 fall through to the generic `ApiFailure.InvalidResponse`, which would have blocked TASK-007's planned `conflict`/`unprocessable` UDF states. Added `ApiFailure.Conflict`/`ApiFailure.Unprocessable` and extended `GiteaRepositoryBrowserApiTest`'s status-mapping test. + - P3 (recorded, not fixed): `CodeBrowserViewModel` runs JSON parsing, Base64 decoding, directory sort/dedupe, and the TASK-006A classifier on `viewModelScope`'s Main dispatcher after the network suspend point resumes. Not blocking; recommended for TASK-016 (release hardening) given its low-end-device and large-content scope. + - Everything else checked (races/cancellation, root back via `BackHandler` in both screens, immutable snapshot wrapping, page-overflow bounds, URL component encoding) verified correct with no finding. +- Checks and results after the fix: `assembleDebug testDebugUnitTest lintDebug --rerun-tasks` BUILD SUCCESSFUL, 102 unit tests across 23 suites, 0 failures/errors, 0 lint errors; `git diff --check` clean; sensitive-pattern scan clean (one match was a test asserting `client_secret` absence, not a leak). +- Task evidence updated: TASK-004, TASK-005, and TASK-006 each record this closure. TASK-005's "reviewed completion commit is TASK-006's contract-freeze base" and TASK-006's "evidence records the exact reviewed TASK-005 contract-freeze commit" checkboxes are both closed. TASK-009's corresponding freeze checkbox remains open pending its own commit and review. +- Known limitations: No physical device was available, so device-interaction checks named in TASK-005/006 remain deferred, unchanged from before this review. +- Next recommended task: TASK-007 (history and change UI), per the ordered backlog in `handoff/claude-master-handoff.md`, once TASK-009 lands and is reviewed; TASK-010 (work-item API contracts) ownership is transferred to Claude per the same handoff. +- Next owner: Claude, continuing the primary implementer role with handback to Codex available at any time by flipping a task's Owner field back and picking up from its last recorded commit. diff --git a/tasks/004-gitea-api-client.md b/tasks/004-gitea-api-client.md index 8b80d2a..73fb131 100644 --- a/tasks/004-gitea-api-client.md +++ b/tasks/004-gitea-api-client.md @@ -1,6 +1,6 @@ # TASK-004: Implement the read-only Gitea API client -Status: implemented and verified — independent review deferred while Codex owns the serial workstream +Status: implemented and independently reviewed by Claude; one P2 fixed (409/422 typed failures) Owner: Codex Reviewer: Claude @@ -45,3 +45,12 @@ Connect the repository browsing domain to authenticated, paginated Gitea API cal - Added immutable `TagSummary` and tag-list domain/use-case contract. - Focused adapter tests cover mapping, pagination, component encoding, authenticated headers, strict file decoding, malformed responses, 404, 429, and local HTTP transport behavior. - Full debug build, unit tests, lint, diff check, and sensitive-pattern scan are recorded in `docs/development-log.md`. + +## Claude independent review closure + +Reviewed read-only at commit `8a47d30` (audited together with TASK-005 `bbbff77` and TASK-006 `16c46a5` per `handoff/claude-master-handoff.md`'s review-and-freeze instruction). Independently reran `assembleDebug testDebugUnitTest lintDebug --rerun-tasks` at `16c46a5`: BUILD SUCCESSFUL, 102 unit tests across 23 suites, 0 failures/errors, 0 lint errors; `git diff --check` clean; sensitive-pattern scan clean (one match was the test itself asserting `client_secret` is absent from the authorization URI). + +- P2 (fixed): `HttpTransport.kt:94-102` `requireSuccess()` mapped only 401/403/404/429/5xx to typed `ApiFailure`; HTTP 409 and 422 fell through to the generic `InvalidResponse`, losing the distinction the review checklist and the upcoming TASK-007 UDF error states (`conflict`/`unprocessable`) require. Fixed by adding `ApiFailure.Conflict` (409) and `ApiFailure.Unprocessable` (422) and extending `GiteaRepositoryBrowserApiTest`'s status-mapping test to cover both. Closure: full gates rerun and passing (above); TASK-007 can now type its conflict/unprocessable states directly from `core/network`. +- URL encoding (verified, no finding): owner/name/path segments use `HttpUrl.Builder.addPathSegment` individually (`repoUrl`, `RepositoryPath.segments()`); `ref` and `path`-filter values use `addQueryParameter`. Confirmed by `GiteaRepositoryBrowserApiTest`'s encoding assertions on repository names containing spaces/`+`/`#` and refs containing `+`/`/`. +- Pagination/page overflow (verified, no finding): `listTree`'s local pagination uses `Long` arithmetic before coercing to `Int`, so no overflow on large `page` values; `Int.MAX_VALUE` page numbers are rejected via `safeNextPage()` instead of wrapping. +- No P0/P1 found. No other P2/P3 raised against this task's scope. diff --git a/tasks/005-repository-branch-ui.md b/tasks/005-repository-branch-ui.md index 1eafe0b..7f3ef19 100644 --- a/tasks/005-repository-branch-ui.md +++ b/tasks/005-repository-branch-ui.md @@ -1,6 +1,6 @@ # TASK-005: Browse repositories and branches -Status: implemented and locally verified — independent review and device check deferred +Status: implemented and independently reviewed by Claude; no P0/P1 findings; device check deferred Owner: Codex Reviewer: Claude @@ -28,7 +28,7 @@ A signed-in user can browse every repository and branch visible to their Gitea a - [x] Retry and pagination cannot create duplicate items or overlapping requests. - [x] Presentation and ViewModel tests cover primary states and selection behavior. - [x] Resource names use the `repositories_` prefix. -- [ ] The reviewed completion commit is recorded as TASK-006's contract-freeze base. +- [x] The reviewed completion commit is recorded as TASK-006's contract-freeze base. ## Evidence @@ -38,3 +38,14 @@ A signed-in user can browse every repository and branch visible to their Gitea a - Repository-wide `assembleDebug testDebugUnitTest lintDebug` passed: 96 unit tests across 21 suites, zero failures/errors. - `git diff --check` passed. Focused sensitive-pattern inspection found no token, client secret, password, private key, or credential material. - The independent reviewer and physical-device interaction check are deferred while Codex owns the serial implementation stream. TASK-006 may be developed on the local contract, but its eventual integration review must include this commit before the contract-freeze checkbox is closed. + +## Claude independent review closure + +Reviewed read-only at commit `bbbff77` per `handoff/claude-master-handoff.md`. Independently reran `assembleDebug testDebugUnitTest lintDebug --rerun-tasks` at `16c46a5` (which includes this commit): BUILD SUCCESSFUL, 102 tests / 23 suites, 0 failures; `git diff --check` clean. + +- Races/cancellation (verified, no finding): `selectRepository`, `refreshReferences`, and `retry` all route through `startOperation`, which cancels the prior `Job` before launching the next one, so rapid repository/branch/tag switches cannot overlap. +- Immutable snapshots (verified, no finding): `RepositoryListUiState.Content` and `ReferencesContent` wrap their lists in `Collections.unmodifiableList`; `backToRepositories()` restores the stored `repositoryContent` snapshot rather than reloading, and the restored snapshot cannot be mutated by later reference-loading state. +- Page overflow (verified, no finding): `loadAllPages` bounds branch/tag pagination to `MAX_REFERENCE_PAGES` (100) and throws a typed failure instead of looping forever if a server never reports the end of a list. +- No P0/P1 found. The 409/422 typed-failure gap recorded under TASK-004 was the only cross-cutting finding and is now fixed there. + +Device interaction remains deferred (no physical device available in this environment); this is an existing known limitation, not a new finding. diff --git a/tasks/006-code-browser.md b/tasks/006-code-browser.md index 4b0b8bb..fab46a7 100644 --- a/tasks/006-code-browser.md +++ b/tasks/006-code-browser.md @@ -1,8 +1,8 @@ # TASK-006: Browse trees and source code by ref -Status: implemented and locally verified — independent review/device check deferred +Status: implemented and independently reviewed by Claude; no P0/P1 findings; device check deferred Owner: Codex -Reviewer: Claude (deferred until independent review is re-enabled) +Reviewer: Claude ## Outcome @@ -29,7 +29,7 @@ Shared domain, data, Gradle, and manifest contracts remain unchanged. Codex temp - [x] Loading, empty, offline, authentication-expired, not-found, and server-error states are localized and accessible. - [x] Tests cover ref preservation, path navigation, content classification, and size limits. - [x] Resource names use the `codebrowser_` prefix. -- [ ] Evidence records the exact reviewed TASK-005 contract-freeze commit. +- [x] Evidence records the exact reviewed TASK-005 contract-freeze commit. ## Evidence @@ -39,3 +39,15 @@ Shared domain, data, Gradle, and manifest contracts remain unchanged. Codex temp - Source viewing reuses the reviewed TASK-006A strict classifier with 1 MB input, 200,000-code-point, and 10,000-line display limits. Text is selectable and copyable; truncation, binary, LFS, unsupported UTF-8, oversized, symlink, and submodule states are explicit. - Focused code-browser tests and repository-wide `assembleDebug testDebugUnitTest lintDebug` passed: 102 unit tests across 23 suites, zero failures. `git diff --check` and focused sensitive-pattern/read-only scans passed. - Device interaction and independent review remain deferred while Codex owns the serial implementation stream. + +## Claude independent review closure + +Reviewed read-only at commit `16c46a5` per `handoff/claude-master-handoff.md`. Independently reran `assembleDebug testDebugUnitTest lintDebug --rerun-tasks`: BUILD SUCCESSFUL, 102 tests / 23 suites, 0 failures; `git diff --check` clean. + +- Root back (verified, no finding): `navigateUp()` returns `false` at the root path instead of calling `loadDirectory`; `MainActivity` treats a `false` result from `onBack` as "leave the code browser" (`codeBrowserViewModel.close(); browsingCode = false`), and both `CodeBrowserScreen` and `RepositoryListScreen` register `BackHandler(onBack = ...)`, so the hardware/gesture back button reaches this logic rather than exiting the Activity directly. +- Rotation/state restoration (partially verified, tracked not new): the `browsingCode` screen-mode flag survives configuration change via `rememberSaveable`, and `CodeBrowserViewModel`/`RepositoryListViewModel` survive rotation as standard `ViewModel`s. Full typed back-stack/process-death restoration is explicitly out of this task's scope and already tracked as `handoff/claude-master-handoff.md`'s TASK-013; not re-raised here. +- Immutable snapshots (verified, no finding): `CodeBrowserUiState.Directory.entries` is wrapped in `Collections.unmodifiableList`. +- P3 (recorded, not fixed): `loadDirectory`/`loadFile` run entirely on `viewModelScope`'s default `Dispatchers.Main.immediate` after the suspending network call resumes — JSON parsing, Base64 decoding, directory sort/dedupe, and `SourceContentClassifier.classify()` (up to 1 MB/200,000 code points) all execute on the main thread. Nothing in this bound is large enough to reliably cause dropped frames, and `AGENTS.md`'s "never block main thread" rule targets network/disk I/O specifically (already off-thread via `OkHttpTransport`'s async callback), so this is not a P0-P2. Recommend wrapping the parse/classify/sort step in `withContext(Dispatchers.Default)` before TASK-016 (release hardening), since that task explicitly targets low-end Android 9 devices and large content. Closure condition: addressed in TASK-016 or a dedicated follow-up, whichever comes first. +- No P0/P1 found. + +Device interaction remains deferred (no physical device available in this environment); this is an existing known limitation, not a new finding.