Skip to content

fix(security): require signed tenant claims on admin endpoints - #268

Draft
seonghobae wants to merge 32 commits into
fix/pii-logging-16240128950440010639from
fix/admin-endpoint-auth-clean
Draft

fix(security): require signed tenant claims on admin endpoints#268
seonghobae wants to merge 32 commits into
fix/pii-logging-16240128950440010639from
fix/admin-endpoint-auth-clean

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Security objective

Make every administrative conversion-job endpoint fail closed under signed tenant claims, least-privilege permissions, tenant-scoped persistence queries, atomic tenant-scoped mutations, immutable job identity, privacy-safe audit evidence, and restart-safe artifact deletion.

Exact bounded stack

Exact current head is 2e8e5ad46da7bb648e9e3f8a394c29d158345c5e on authoritative parent #270 exact head c1239e20b048a582b19cb9eeb90f2c29f7c8f814.

The effective stacked comparison currently contains 32 commits and 72 changed files. Parent #270 must integrate first; this draft must then be reconciled onto the protected current main head and reverified. No parent, predecessor, snapshot-base, or synthetic-merge result transfers across a changed head.

Security and lifecycle boundary

  • Require admin:read for tenant-scoped listing and admin:write for delete and retry.
  • Fail closed before repository access when signed-claim verification is absent, malformed, expired, weakly configured, or missing the required permission.
  • Select and mutate only through tenant-scoped repository and state-store contracts; missing and cross-tenant objects remain indistinguishable.
  • Permanently reserve accepted conversion-job UUIDs, reject distinct live or tombstoned reuse, and bind queued retry work to immutable tenant and generation evidence.
  • Serialize primary records, identifier reservations, tenant/content indexes, conversion writes, and deletion work at the lifecycle boundary.
  • Emit domain-separated HMAC fingerprints without raw actor, tenant, job, signature, filename, path, document, or exception-controlled values.
  • Source signing and audit secrets through the shared config-tree boundary and enforce key strength and separation.
  • Route production deletion through the primary durable deletion service: persist receipt-first metadata tombstones, retain failed artifact cleanup as retryable evidence, replay bounded work after startup and on schedule, validate exact artifact digest/generation, and expose low-cardinality cleanup metrics.

Test-first evidence

The slice includes deterministic unit, integration, security, concurrency, restart-replay, privacy, coverage, and compatibility regressions for tenant-crossing retry races; UUID replacement and tombstone collisions; tenant-bound secondary-index ownership; delete/save interleavings; scoped-adapter defaults; null, missing, and cross-tenant concealment; config-tree secret sourcing; audit-log exclusions; durable deletion failure and recovery; ledger corruption and illegal transition rejection; exact attempt evidence; and terminal-state replay rejection.

Exact-head acceptance state

For exact current head 2e8e5ad46da7bb648e9e3f8a394c29d158345c5e:

  • CI run 31106968192: success, including exact-head Maven verify, synthetic merge compatibility, buyer-readiness script tests, 604 tests with zero failures/errors/skips, zero missed production statements and branches, and warning-free public Javadocs;
  • fuzz run 31106968335: success for tenant claims, document validation, and artifact-token parsing;
  • CodeRabbit commit status: success, but status-only evidence is not a formal review or counted approval;
  • unresolved inline review threads: zero;
  • Security Scan, SAST Semgrep, and Strix on this exact head: absent and therefore not passing;
  • formal exact-current-head CodeRabbit/OpenCode/Noema/Strix review evidence: absent;
  • counted independent write-authorized approval: absent.

Keep this PR Draft. Merge order remains #270 → protected-head reconciliation and full exact-head gates for this slice. Do not weaken tests, restore tenant fallbacks, permit UUID reuse, add global mutation fallbacks, bypass protections, or publish a release before integrated release acceptance succeeds.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

테넌트 관리자 보안

Layer / File(s) Summary
서명 인증 및 감사 계약
AGENTS.md, docs/deployment/..., docs/security/..., src/main/java/com/clearfolio/viewer/auth/*, src/main/java/com/clearfolio/viewer/audit/*, src/main/java/com/clearfolio/viewer/security/*, src/test/java/com/clearfolio/viewer/audit/*, src/test/java/com/clearfolio/viewer/auth/*, src/test/java/com/clearfolio/viewer/config/*
관리자 요청은 강한 HMAC 기반 signed tenant claims와 ADMIN_READ 또는 ADMIN_WRITE 권한을 사용합니다. 감사 로그는 actor·tenant·job 식별자를 도메인별 HMAC fingerprint로 기록합니다. tenant claims secret은 config-tree mount에서 로드합니다.
테넌트 범위 저장소 및 원자적 상태 변경
src/main/java/com/clearfolio/viewer/repository/*, src/main/java/com/clearfolio/viewer/service/*, src/test/java/com/clearfolio/viewer/repository/*, src/test/java/com/clearfolio/viewer/service/*
저장소와 서비스가 tenant-scoped 조회·삭제·dead-letter 재시도를 제공합니다. 삭제 후에만 artifact를 정리하고, 승인된 재시도만 worker에 enqueue합니다. primary map과 content-hash index 갱신을 직렬화하고 stale index와 UUID 충돌을 fail-closed 처리합니다.
관리자 엔드포인트 연결 및 검증
src/main/java/com/clearfolio/viewer/controller/AdminController.java, src/test/java/com/clearfolio/viewer/controller/*
목록·삭제·재시도 API가 signed headers와 tenant context를 사용합니다. 누락 대상은 404, 부적격 재시도는 409, 승인된 재시도는 202로 매핑합니다. 인증 실패와 작업 결과를 감사 로그에 기록합니다.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

Possibly related PRs

Sequence Diagram(s)

sequenceDiagram
  participant AdminClient
  participant AdminController
  participant TenantAccessService
  participant DefaultDocumentConversionService
  participant InMemoryConversionJobRepository
  participant ConversionJobStateStore
  participant AdministrativeAuditLogger

  AdminClient->>AdminController: signed tenant claims and admin permission
  AdminController->>TenantAccessService: requireSigned
  TenantAccessService-->>AdminController: TenantContext
  AdminController->>DefaultDocumentConversionService: tenant-scoped operation
  DefaultDocumentConversionService->>InMemoryConversionJobRepository: tenant and job identifier
  InMemoryConversionJobRepository-->>DefaultDocumentConversionService: scoped result
  DefaultDocumentConversionService->>ConversionJobStateStore: tenant-bound retry transition
  ConversionJobStateStore-->>DefaultDocumentConversionService: retry outcome
  AdminController->>AdministrativeAuditLogger: action, outcome, HTTP status
  AdminController-->>AdminClient: scoped HTTP response
Loading
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning 대부분의 요구사항을 구현했지만, 재시도와 동일 UUID 교체의 동시성 경합을 해결하지 않아 핵심 보안 요구사항을 충족하지 못합니다 [#266]. 서로 다른 작업의 동일 UUID 교체를 거부하거나 불변 tenant/generation 디스패치 계약을 도입하고 결정적 회귀 테스트를 추가하십시오.
Docstring Coverage ⚠️ Warning Docstring coverage is 28.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed 구현, 설정, 문서, OpenAPI 계약 및 테스트 변경은 관리자 인가와 테넌트 범위 보안 목표에 직접 관련됩니다.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 관리자 엔드포인트에 서명된 테넌트 클레임을 요구하는 핵심 변경을 명확하고 간결하게 설명합니다.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/admin-endpoint-auth-clean

Comment @coderabbitai help to get the list of available commands.

@seonghobae
seonghobae changed the base branch from fix/pii-logging-clean to fix/pii-logging-16240128950440010639 August 5, 2026 13:09

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Rebuild this stacked PR cleanly on authoritative parent #270 exact head 06452c3f39f2deb38d31d189e46de4b25512baa0. Current implementation head 808e57d6006f1ac332e98ec32d7c156ded4f48dd is a divergent descendant of the retired parent history and is not merge evidence.

Preserve only the bounded administrative-authorization slice represented by the current 31-file diff: signed-claim verification, least-privilege admin:read/admin:write, tenant-scoped list/delete/retry contracts, concealed missing/cross-tenant outcomes including null UUIDs, atomic repository/state-store mutations, secondary-index consistency, privacy-safe domain-separated administrative audit evidence, config-tree secret loading, OpenAPI/deployment contract, realistic concurrency/security tests, and authoritative documentation. Reconcile shared CHANGELOG.md, AuditPseudonymizer.java, and AuditPseudonymizerTest.java against parent #270 rather than overwriting its Netty evidence. Do not add temporary workflows, repair scripts, unrelated dependency changes, or predecessor-parent files.

Use a clean parent-based branch history or replace the current branch only after the complete tree is ready. Run the strongest available local mvn -B --no-transfer-progress verify and relevant contract tests, keep the PR draft, and report the exact new head. After the clean rebuild, require fresh exact-head CI/security/fuzz/review evidence when the stack can target protected main; do not count current divergent-head or predecessor results.

@seonghobae
seonghobae changed the base branch from fix/pii-logging-16240128950440010639 to fix/pii-logging-clean August 5, 2026 13:11
@seonghobae
seonghobae changed the base branch from fix/pii-logging-clean to fix/pii-logging-16240128950440010639 August 5, 2026 13:15

Copy link
Copy Markdown
Collaborator Author

@opencode-agent @cwl-noema-review Review exact stacked head 808e57d6006f1ac332e98ec32d7c156ded4f48dd against parent #270 exact 06452c3f39f2deb38d31d189e46de4b25512baa0 without treating absent Actions as passing.

Manual review found that both ConversionJobRepository.findByTenantAndId and findByTenantAndContentHash retained adapter defaults that called global lookup and filtered only after materializing a job. That contradicted the branch's fail-closed modular-adapter contract. Test-first commits now prove neither global lookup path is invoked; both defaults return empty until a durable adapter implements scoped predicates, and InMemoryConversionJobRepository explicitly implements owned identifier and tenant+hash lookup. Verify the tests, Javadocs, shared-lock usage, null/blank/missing/cross-tenant concealment, and that no legitimate standalone or administrative caller silently relies on the old global fallback. Update the authoritative administrative-authorization documentation and one existing CHANGELOG.md Security entry if wording is incomplete, without duplicating sections.

Keep Draft. Parent #270 must integrate first; then this branch must be reconciled onto protected main and rerun exact-head Maven verify, zero missed line/branch coverage, warning-free Javadocs, CI, Security, SAST, fuzz, CodeRabbit, Strix/OpenCode/Noema, unresolved-thread, independent-approval, and branch-protection gates.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Parent update: rebuild the tenant-scoped administrative authorization slice directly on #270 exact head 91091ddc212dac328ff36696f56c0a15c21407f4, not retired parent 06452c3f39f2deb38d31d189e46de4b25512baa0. Preserve the bounded 31-file security slice only: signed claims, least-privilege admin:read/admin:write, tenant-scoped list/delete/retry, concealed missing/cross-tenant/null-UUID outcomes, atomic state transitions, tenant-bound secondary indexes, domain-separated privacy-safe audit evidence, config-tree secrets, deployment/OpenAPI contracts, and security/concurrency tests. Reconcile shared AGENTS.md, CHANGELOG.md, AuditPseudonymizer, DefaultDocumentConversionService, InMemoryConversionJobRepository, and their tests against the parent rather than overwriting #270's audit-key, Netty/SBOM, filesystem TOCTOU, zero-coverage, or warning-free-Javadoc contracts. Use a clean parent-based history, keep the PR draft, run mvn -B --no-transfer-progress verify, and report the exact rebuilt head. Do not add temporary/write-scoped workflows or count current divergent-head/predecessor evidence.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent @cwl-noema-review Rebuild this draft as a clean descendant of authoritative parent #270 exact head 26563218ae42eaa876c784fcf56b27f8cb810080.

Fresh GitHub comparison shows current head 808e57d6006f1ac332e98ec32d7c156ded4f48dd is diverged: 53 commits ahead, 84 behind, merge base 5261356ac34e6545bce947ba0bcf2b1ce9f9be67. Preserve only the bounded 31-file administrative-authorization slice: signed tenant claims; least-privilege admin:read/admin:write; tenant-scoped list/delete/retry; atomic owned mutations; tenant-bound indexes; null/missing/cross-tenant concealment; privacy-safe domain-separated audit evidence; config-tree secret loading; deployment/OpenAPI contracts; deterministic security and concurrency tests; and authoritative documentation.

Reconcile shared CHANGELOG.md, AuditPseudonymizer.java, and AuditPseudonymizerTest.java without overwriting the parent's privacy, Netty 4.1.136.Final, deterministic SBOM/attribution, exact-head, zero-missed-line/branch, warning-free Javadoc, and fail-closed Maven report-evidence contracts. Do not add temporary workflows, repair scripts, unrelated dependencies, release changes, or predecessor-parent files.

Implement test-first, run the strongest available mvn -B --no-transfer-progress verify and relevant contract tests, keep the PR draft, and report the exact rebuilt head. Do not count the current CodeRabbit status or absent Actions as acceptance evidence.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Reconstruct this draft as a clean descendant of authoritative parent exact head 26563218ae42eaa876c784fcf56b27f8cb810080 now, without waiting for #270 to merge and without introducing a replacement PR or temporary workflow.

Required bounded operation:

  1. Preserve only the intended administrative tenant-authorization slice described in this PR: signed-claim fail-closed construction, admin:read/admin:write, tenant-scoped list and atomic delete/retry contracts, tenant-bound secondary indexes, privacy-safe administrative audit evidence, and their tests/docs.
  2. Rebase/rebuild the branch so the parent exact SHA is its merge base and the branch is 0 commits behind that parent. Do not carry predecessor fix(security): pseudonymize policy override audit identifiers #267/fix(security): require signed tenant claims on admin endpoints #266 history, stale SBOM copies, earlier Netty commits, one-shot repair workflows, dependency drift, or unrelated release/automation changes.
  3. Reconcile—not overwrite—the parent's AuditPseudonymizer, AuditPseudonymizerTest, CHANGELOG.md, privacy documentation, Netty 4.1.136.Final, deterministic SBOM/attribution, strict Maven report-evidence parser, exact-head/synthetic-merge CI, zero-missed line/branch coverage, and warning-free Javadoc contracts.
  4. Preserve the recently verified missing-UUID fail-closed behavior for scoped lookup/delete/retry and all concurrency/index ownership regressions.
  5. Run mvn -B --no-transfer-progress verify, python -m pytest -q scripts, and the authoritative dependency/evidence checks on the reconstructed head. No skipped tests, coverage relaxation, fallback global lookup, legacy unscoped mutation, or raw identifier/token logging.
  6. Keep the PR draft. Report the exact new head, parent comparison, bounded changed-file list, test totals, coverage/Javadoc evidence, and absence of temporary/write-scoped workflows. Request fresh CodeRabbit/OpenCode/Noema review only after the branch is clean and same-head checks are available.

Do not merge, retarget to main, publish, release, or bypass the independent approval gate.

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@opencode-agent @cwl-noema-review Please independently review exact current head fe2e53328a0b44c1c449d724314c6c4944eb572b only, treating the divergent predecessor head and all predecessor results as stale.

This draft is now a clean descendant of parent #270 exact 26563218ae42eaa876c784fcf56b27f8cb810080, 2 commits ahead and 0 behind, mergeable, with the same bounded 31-file administrative-authorization slice. Verify signed tenant claims, strong-verifier fail-closed behavior, admin:read/admin:write least privilege, tenant-scoped list/delete/retry boundaries, concealed missing/null/cross-tenant outcomes, fail-closed adapter defaults without global lookup or legacy mutation, atomic repository and state-store behavior, secondary-index collision/replacement/concurrency invariants, domain-separated audit pseudonyms, config-tree secret sourcing, OpenAPI/deployment contracts, complete public Javadocs, and preservation of every #270 privacy/Netty/SBOM/report-evidence contract.

The exact head has successful fuzz run 31067103470 and successful CodeRabbit commit status; unresolved inline threads are zero. Repository-local CI, Security Scan, and SAST did not run for the stacked base, so they are explicitly absent-required rather than passing. Do not substitute parent or predecessor results. A full exact-head acceptance run remains mandatory after #270 integrates and the bounded slice targets protected main.

Submit actionable path-specific findings or a formal exact-head review after your own inspection. Parent integration and a counted independent write-authorized approval remain separate mandatory gates.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

@seonghobae I will review PR #268 at exact head fe2e53328a0b44c1c449d724314c6c4944eb572b.

I will treat predecessor-head results as stale. I will treat absent CI, Security Scan, and SAST results as not passing.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@opencode-agent @cwl-noema-review Independently review exact current head fe2e53328a0b44c1c449d724314c6c4944eb572b relative to parent #270 exact 26563218ae42eaa876c784fcf56b27f8cb810080. The clean stack is 2 commits ahead, 0 behind, mergeable, and contains exactly the bounded 31-file administrative-authorization slice.

Verify fail-closed signed-claim construction and freshness/signature/permission checks; admin:read/admin:write; tenant-scoped listing before object materialization; repository/state/service compatibility defaults that never call global lookup or legacy mutations; atomic tenant-scoped delete/retry ordering; null-ID/missing/cross-tenant concealment; tenant-bound content-hash index ownership and one-critical-section concurrency; UUID collision/replacement/stale-index/monitor races; exact enqueue and artifact-cleanup conditions; domain-separated actor and tenant audit pseudonyms without raw claims, subject, tenant, filename, content, or signatures; config-tree-only secret sourcing; and beginner-readable public Javadocs.

The branch currently has successful exact-head fuzz run 31067103470; CI, Security Scan, and SAST are absent for this stacked base and must not be inferred from parent or predecessor evidence. Keep Draft behind #270, report exact-head findings or advisory evidence, and do not infer counted independent approval.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='fe2e53328a0b44c1c449d724314c6c4944eb572b'
parent='26563218ae42eaa876c784fcf56b27f8cb810080'

printf '%s\n' '== Current commit and parent relationship =='
git show -s --format='HEAD %H%nParents %P%nSubject %s' "$head"
printf 'merge-base '
git merge-base "$head" "$parent"
printf 'ahead/behind '
git rev-list --left-right --count "$parent...$head"

printf '%s\n' '== Parent-relative changed paths =='
git diff --name-status "$parent" "$head"

printf '%s\n' '== Changed-file structural outline =='
git diff --name-only "$parent" "$head" | while IFS= read -r path; do
  case "$path" in
    *.java)
      printf '\n--- %s ---\n' "$path"
      ast-grep outline "$path" --items all --view compact 2>/dev/null || true
      ;;
  esac
done

printf '%s\n' '== Security boundary symbols in changed files =='
git diff --name-only "$parent" "$head" | while IFS= read -r path; do
  case "$path" in
    *.java|*.yml|*.yaml|*.properties|*.md|*.json)
      rg -n -i -C 2 \
        'findAllByTenantId|findByTenantAnd(Id|ContentHash)|deleteByTenantAndId|retryDeadLetteredForTenant|TenantContext|admin:(read|write)|AuditPseudonymizer|HMAC|secret|X-Clearfolio|content.?hash' \
        "$path" || true
      ;;
  esac
done

Length of output: 89305


🏁 Script executed:

#!/bin/bash
set -euo pipefail

print_range() {
  local path="$1" start="$2" end="$3"
  printf '\n===== %s, lines %s-%s =====\n' "$path" "$start" "$end"
  sed -n "${start},${end}p" "$path" | nl -ba -v "$start"
}

print_range src/main/java/com/clearfolio/viewer/auth/TenantAccessService.java 65 190
print_range src/main/java/com/clearfolio/viewer/controller/AdminController.java 45 285
print_range src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java 1 230
print_range src/main/java/com/clearfolio/viewer/repository/ConversionJobRepository.java 1 175
print_range src/main/java/com/clearfolio/viewer/repository/ConversionJobStateStore.java 1 135
print_range src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java 1 385
print_range src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java 210 335
print_range docs/deployment/clearfolio-buyer-connector.openapi.yaml 75 140
print_range docs/deployment/clearfolio-buyer-connector.openapi.yaml 250 390

printf '\n===== Administrative mappings and legacy permission/header references =====\n'
rg -n -C 3 \
  '@(GetMapping|DeleteMapping|PostMapping)|admin:(read|write)|job:retry|X-Clearfolio-Operator-Id|retryDeadLetteredConversionJob' \
  src/main/java docs/deployment/clearfolio-buyer-connector.openapi.yaml

printf '\n===== Audit logger call sites and logging statements =====\n'
rg -n -C 3 \
  'administrativeAuditLogger|auditLogger|logger\.(info|warn|error|debug|trace)|recordDenied' \
  src/main/java/com/clearfolio/viewer/controller/AdminController.java \
  src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java

Length of output: 2090


🏁 Script executed:

#!/bin/bash
set -euo pipefail

print_range() {
  local path="$1" start="$2" end="$3"
  printf '\n===== %s, lines %s-%s =====\n' "$path" "$start" "$end"
  sed -n "${start},${end}p" "$path"
}

print_range src/main/java/com/clearfolio/viewer/auth/TenantAccessService.java 65 190
print_range src/main/java/com/clearfolio/viewer/controller/AdminController.java 45 285
print_range src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java 1 230
print_range src/main/java/com/clearfolio/viewer/repository/ConversionJobRepository.java 1 175
print_range src/main/java/com/clearfolio/viewer/repository/ConversionJobStateStore.java 1 135
print_range src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java 1 385
print_range src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java 210 335
print_range docs/deployment/clearfolio-buyer-connector.openapi.yaml 75 140
print_range docs/deployment/clearfolio-buyer-connector.openapi.yaml 250 390

printf '\n===== Administrative mappings and legacy permission/header references =====\n'
rg -n -C 3 \
  '@(GetMapping|DeleteMapping|PostMapping)|admin:(read|write)|job:retry|X-Clearfolio-Operator-Id|retryDeadLetteredConversionJob' \
  src/main/java docs/deployment/clearfolio-buyer-connector.openapi.yaml

printf '\n===== Audit logger call sites and logging statements =====\n'
rg -n -C 3 \
  'administrativeAuditLogger|auditLogger|logger\.(info|warn|error|debug|trace)|recordDenied' \
  src/main/java/com/clearfolio/viewer/controller/AdminController.java \
  src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java

Length of output: 82816


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '===== Worker enqueue and job lookup implementations ====='
rg -n -C 5 \
  '\benqueue\s*\(|\bclaimForProcessing\s*\(|\bfindById\s*\(' \
  src/main/java

printf '%s\n' '===== Retry, replacement, and stale-observation test coverage ====='
rg -n -i -C 6 \
  'retry.*(replacement|concurrent|race|stale)|replacement.*retry|same.?UUID|sharedJobId|computeIfPresent|enqueue' \
  src/test/java/com/clearfolio/viewer

printf '%s\n' '===== Full tenant retry and concurrency test files ====='
for path in \
  src/test/java/com/clearfolio/viewer/service/TenantScopedRetryContractTest.java \
  src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryConcurrencyTest.java \
  src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryTenantScopeTest.java
do
  printf '\n--- %s ---\n' "$path"
  cat "$path"
done

Length of output: 170541


@seonghobae I found one actionable exact-head issue.

[P1] Serialize tenant retry with same-UUID replacement
retryDeadLetteredForTenant transitions the job through jobs.computeIfPresent, but it does not use jobIndexLock. save can replace the same UUID under jobIndexLock. After the retry reports ACCEPTED, DefaultDocumentConversionService enqueues only the UUID. DefaultConversionWorker later resolves that UUID through an unscoped findById.

A concurrent sequence can therefore occur:

  1. Tenant A retries its dead-lettered job.
  2. The repository changes Tenant A's job to SUBMITTED.
  3. A save replaces the UUID with Tenant B's job.
  4. The retry path enqueues the shared UUID.
  5. The worker resolves and processes Tenant B's replacement.

This breaks the tenant-bound retry invariant. It can also process a replacement that the retry caller did not authorize. Use the same critical section for the tenant-scoped retry transition and same-UUID replacement. Add a deterministic test that forces replacement after the retry transition but before enqueue or worker lookup.

Affected code: src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java, Line 305; src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java, Line 261; src/main/java/com/clearfolio/viewer/service/DefaultConversionWorker.java, Line 196.

I verified the stated parent relationship and inspected the signed-claim, least-privilege, fail-closed adapter-default, list, delete, audit, secret-source, and index boundaries. CI, Security Scan, and SAST remain absent-required. I did not treat them as passing.

You are interacting with an AI system.

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@opencode-agent @cwl-noema-review Independently inspect exact current head eea9c4c4442742d314bf532736f067e905a1e1f3 relative to parent #270 exact 26563218ae42eaa876c784fcf56b27f8cb810080.

Two review-discovered defects were fixed test-first and must be verified, not assumed:

  1. The clean reconstruction had reintroduced stale AGENTS.md guidance that weakened the authoritative mvn -B --no-transfer-progress verify, zero-skips, exact-head, Netty, JaCoCo, and warning-free public-Javadoc contracts and restored obsolete sibling names. The current diff must retain every parent gate and current CWL name, changing only the valid tenant-claims config-tree guidance.
  2. Administrative audit logs emitted raw conversion-job UUIDs. Test commit 0e57c13401df51967c644980b4bb8d0d5db82cb9 requires jobFingerprint and forbids both the raw UUID and jobId=. Verify the production job-specific HMAC domain, actor/tenant/job domain separation, absent/unavailable markers, beginner-readable Javadocs, captured-log exclusions, and the updated security decision.

Run mvn -B --no-transfer-progress verify and python -m pytest -q scripts on this exact head. Report exact test totals, zero failures/errors/skips, zero missed production lines/branches, warning-free Javadocs, changed-file scope, and any actionable path-specific finding. The exact-head fuzz run is still pending; CI, Security Scan, and SAST are absent on the stacked base and must not be inferred from parent or predecessor evidence. Keep Draft, do not merge, and do not infer counted independent approval.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='eea9c4c4442742d314bf532736f067e905a1e1f3'
parent='26563218ae42eaa876c784fcf56b27f8cb810080'

echo '== Exact commit relationship =='
git show -s --format='HEAD %H%nParents %P%nSubject %s' "$head"
printf 'merge-base: '; git merge-base "$head" "$parent"
printf 'ahead/behind: '; git rev-list --left-right --count "$parent...$head"

echo '== Parent-relative changed paths =='
git diff --name-status "$parent" "$head"

echo '== AGENTS.md parent-relative diff =='
git diff --unified=80 "$parent" "$head" -- AGENTS.md

echo '== Audit-related changed paths =='
git diff --name-only "$parent" "$head" | rg -i '(audit|admin|security|decision|pseudonym|test)' || true

echo '== Audit source/test structural outlines =='
fd -t f -i 'AuditPseudonymizer|AdministrativeAuditLogger' src | while IFS= read -r f; do
  echo "--- $f ---"
  ast-grep outline "$f" --items all --view compact || true
done

echo '== Job fingerprint, domains, raw jobId logging, and captured-log assertions =='
rg -n -i -C 4 \
  'jobFingerprint|conversion[- ]job|jobId=|jobId\b|HMAC|domain|unavailable|absent|captur|appender' \
  src/main/java src/test/java docs AGENTS.md 2>/dev/null || true

echo '== Security decision and current CWL/config-tree terms =='
rg -n -i -C 3 \
  'administrative|audit|pseudonym|config[- ]tree|CWL|Netty|JaCoCo|javadoc|exact.head|zero.skip|verify' \
  AGENTS.md docs src/main/resources src/test/java 2>/dev/null || true

echo '== Recent exact-head test and production diffs =='
git show --stat --oneline --decorate "$head"
git diff --unified=40 "${head}^..${head}" -- \
  ':(glob)**/*AuditPseudonymizer*.java' \
  ':(glob)**/*AdministrativeAuditLogger*.java' \
  ':(glob)**/*Audit*.java' \
  ':(glob)**/*security*' \
  ':(glob)**/*Security*' \
  ':(glob)**/AGENTS.md' || true

Length of output: 202329


🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='eea9c4c4442742d314bf532736f067e905a1e1f3'
parent='26563218ae42eaa876c784fcf56b27f8cb810080'

echo '== Relationship and changed-file count =='
printf 'merge-base='; git merge-base "$head" "$parent"
printf 'ahead_behind='; git rev-list --left-right --count "$parent...$head"
git diff --name-only "$parent" "$head" | tee /tmp/pr268-changed-files.txt
printf 'changed_file_count='; wc -l < /tmp/pr268-changed-files.txt

echo '== AGENTS.md diff and parent-equivalence outside config-tree terms =='
git diff --unified=12 "$parent" "$head" -- AGENTS.md
echo '-- parent and head AGENTS.md SHA-256 --'
git show "$parent:AGENTS.md" | sha256sum
git show "$head:AGENTS.md" | sha256sum
echo '-- changed AGENTS.md line numbers at head --'
git diff -U0 "$parent" "$head" -- AGENTS.md | rg '^@@|^[+-][^+-]' || true

echo '== Locate administrative audit implementation and decision =='
fd -t f -a . src/main/java src/test/java docs/security | rg \
  '(AdministrativeAuditLogger|AuditPseudonymizer|administrative-authorization)' | sort

echo '== AdministrativeAuditLogger source and test =='
for f in \
  src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java \
  src/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java \
  docs/security/2026-08-05-administrative-authorization.md
do
  if [ -f "$f" ]; then
    echo "--- $f ---"
    nl -ba "$f"
  fi
done

echo '== All AdministrativeAuditLogger call sites with context =='
rg -n -C 6 '\b(record|recordHeaders|jobFingerprint|actorFingerprint|tenantFingerprint)\s*\(' \
  src/main/java/com/clearfolio/viewer \
  -g '*.java'

echo '== Administrative-audit raw identifier fields or logs =='
rg -n -i -C 4 \
  '(jobId=|jobId\b|conversion.?job|logger\.(info|warn|error|debug|trace)|log\.(info|warn|error|debug|trace))' \
  src/main/java/com/clearfolio/viewer/controller/AdminController.java \
  src/main/java/com/clearfolio/viewer/audit \
  src/test/java/com/clearfolio/viewer/audit 2>/dev/null || true

echo '== Parent-relative audit/doc/test diffs =='
git diff --unified=20 "$parent" "$head" -- \
  src/main/java/com/clearfolio/viewer/audit \
  src/test/java/com/clearfolio/viewer/audit \
  docs/security/2026-08-05-administrative-authorization.md

Length of output: 2709


🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='eea9c4c4442742d314bf532736f067e905a1e1f3'
parent='26563218ae42eaa876c784fcf56b27f8cb810080'

echo '== Changed-file count and paths =='
changed_files="$(git diff --name-only "$parent" "$head")"
printf '%s\n' "$changed_files"
printf 'changed_file_count='
printf '%s\n' "$changed_files" | awk 'NF { count += 1 } END { print count }'

echo '== AGENTS.md parent-relative diff =='
git diff --unified=16 "$parent" "$head" -- AGENTS.md

echo '== Relevant source and documentation files =='
fd -t f -a . src/main/java src/test/java docs/security | rg \
  '(AdministrativeAuditLogger|AuditPseudonymizer|administrative-authorization)' | sort

for f in \
  src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java \
  src/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java \
  docs/security/2026-08-05-administrative-authorization.md
do
  if [ -f "$f" ]; then
    echo "--- $f ---"
    nl -ba "$f"
  fi
done

echo '== Administrative logger call sites =='
rg -n -C 6 \
  'administrativeAuditLogger\.(record|recordHeaders)|\b(record|recordHeaders)\s*\(' \
  src/main/java/com/clearfolio/viewer \
  -g '*.java'

echo '== Administrative audit raw-ID and logging references =='
rg -n -i -C 4 \
  '(jobId=|jobId\b|conversion.?job|logger\.(info|warn|error|debug|trace)|log\.(info|warn|error|debug|trace))' \
  src/main/java/com/clearfolio/viewer/controller/AdminController.java \
  src/main/java/com/clearfolio/viewer/audit \
  src/test/java/com/clearfolio/viewer/audit 2>/dev/null || true

echo '== Parent-relative audit, test, and decision diff =='
git diff --unified=24 "$parent" "$head" -- \
  src/main/java/com/clearfolio/viewer/audit \
  src/test/java/com/clearfolio/viewer/audit \
  docs/security/2026-08-05-administrative-authorization.md

Length of output: 8601


🏁 Script executed:

#!/bin/bash
set -euo pipefail

show_file() {
  file="$1"
  if [ -f "$file" ]; then
    echo "--- $file ---"
    sed -n '1,340p' "$file"
  else
    echo "--- missing: $file ---"
  fi
}

show_file src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java
show_file src/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java
show_file src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java
show_file src/test/java/com/clearfolio/viewer/security/AuditPseudonymizerTest.java
show_file docs/security/2026-08-05-administrative-authorization.md

echo '== AdministrativeAuditLogger construction and invocation sites =='
rg -n -C 8 \
  '(new AdministrativeAuditLogger|AdministrativeAuditLogger|administrativeAuditLogger\.(record|recordHeaders))' \
  src/main/java/com/clearfolio/viewer \
  -g '*.java'

echo '== Every direct logger call in the administrative controller and audit package =='
rg -n -C 4 \
  '(logger|log)\.(info|warn|error|debug|trace)\s*\(' \
  src/main/java/com/clearfolio/viewer/controller/AdminController.java \
  src/main/java/com/clearfolio/viewer/audit \
  -g '*.java' || true

echo '== Exact job field names in audit implementation and captured-log tests =='
rg -n -C 5 \
  '(jobFingerprint|jobId=|jobId\b|raw-job|raw.*UUID|UUID)' \
  src/main/java/com/clearfolio/viewer/audit \
  src/test/java/com/clearfolio/viewer/audit \
  docs/security/2026-08-05-administrative-authorization.md \
  -g '*.java' -g '*.md'

echo '== Parent-relative final audit/privacy commits =='
git log --format='%H %s' --ancestry-path \
  26563218ae42eaa876c784fcf56b27f8cb810080..eea9c4c4442742d314bf532736f067e905a1e1f3
git show --format=fuller --stat 0e57c13401df51967c644980b4bb8d0d5db82cb9 \
  2e6622ebe93c7f03bd019fe308aa346cecf8a908 \
  4d453dfdbf76cd3cde2eaafbee875aee92eaa773 \
  eea9c4c4442742d314bf532736f067e905a1e1f3

Length of output: 80286


@seonghobae I inspected exact head eea9c4c4442742d314bf532736f067e905a1e1f3 against parent 26563218ae42eaa876c784fcf56b27f8cb810080.

The merge base is the requested parent. The branch is 7 commits ahead and 0 commits behind. The parent-relative diff contains 31 files.

I verified the two requested corrections:

  • AGENTS.md changes only the tenant-claims secret guidance. It now requires the config-tree mount selected by CLEARFOLIO_SECRET_CONFIG_DIR. The diff does not weaken the parent acceptance-gate guidance.
  • AdministrativeAuditLogger uses jobFingerprint, not jobId=, in its log template. It applies AuditPseudonymizer.forAdministrativeJob(...).
  • AuditPseudonymizer defines separate HMAC domains for administrative actor, tenant, and conversion-job values.
  • AdministrativeAuditLoggerTest captures formatted log output. It rejects both the raw UUID and jobId=.
  • The security decision now explicitly excludes raw job UUIDs and defines the three separate fingerprint domains.

I found one remaining actionable issue from the prior exact-head review.

[P1] Serialize tenant retry with same-UUID replacement
InMemoryConversionJobRepository.retryDeadLetteredForTenant still does not use the shared jobIndexLock. A concurrent save can replace the same UUID after the retry transition and before worker resolution. The retry path then enqueues a UUID that DefaultConversionWorker resolves through unscoped findById. The worker can process the replacement job for another tenant.

The commits after fe2e53328a0b44c1c449d724314c6c4944eb572b do not modify InMemoryConversionJobRepository.java, DefaultDocumentConversionService.java, or DefaultConversionWorker.java. Therefore, this finding remains on exact head eea9c4c4442742d314bf532736f067e905a1e1f3.

Use one shared critical section for same-UUID save and tenant-scoped retry. Add a deterministic test that replaces the UUID after the owned retry transition and before worker lookup.

I could not run mvn -B --no-transfer-progress verify or python -m pytest -q scripts. The sandbox does not permit compiler or interpreter execution. Therefore, test totals, zero failures/errors/skips, zero missed lines/branches, and warning-free Javadocs are unverified. The pending fuzz run, absent CI, Security Scan, and SAST are not passing evidence. I did not approve or merge this draft.

⚠️ Action not completed

Head commit changed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Fix the newly confirmed exact-head P1 retry/replacement race on fe2e53328a0b44c1c449d724314c6c4944eb572b test-first, without broadening this 31-file administrative-authorization slice.

CodeRabbit independently established this concrete sequence:

  1. Tenant A's dead-lettered job transitions to SUBMITTED through retryDeadLetteredForTenant.
  2. Before the service enqueue or worker lookup completes, save replaces the same UUID with Tenant B's job.
  3. The service enqueues only the UUID.
  4. DefaultConversionWorker later resolves that UUID through unscoped findById and can process Tenant B's replacement.

Required bounded correction:

  1. Add a deterministic RED concurrency test that pauses the accepted retry after the authorized transition and forces a same-UUID cross-tenant replacement before enqueue/claim. It must prove the replacement is never enqueued or processed and no unauthorized lifecycle event is emitted.
  2. Make conversion-job identifiers immutable while a live/tombstoned record owns them, or introduce an equivalent immutable tenant+generation dispatch receipt. A lock around only jobs.computeIfPresent is insufficient because replacement can occur after that lock is released and before UUID-only worker resolution.
  3. Prefer the smallest coherent design consistent with issue [Product Gap] Complete tenant-safe document lifecycle and artifact deletion #263's non-reusable identifier/lifecycle-generation contract. save may remain idempotent for the exact same stored object if required, but it must fail closed on a distinct live object with the same UUID; deletion/recreation semantics must be explicit and tested rather than silently replacing ownership.
  4. Rework existing tests that currently normalize direct live same-UUID replacement as valid behavior. Preserve the useful delete-vs-create critical-section test, tenant/hash index consistency, collision detection, missing/null/cross-tenant concealment, and adapter fail-closed defaults.
  5. Ensure tenant-scoped retry, enqueue, and worker claim remain bound to the authorized immutable object/generation. Do not add a global lookup fallback, raw tenant/job logging, temporary workflow, dependency, or unrelated refactor.
  6. Run mvn -B --no-transfer-progress verify, python -m pytest -q scripts, and all repository concurrency/security contracts. Preserve zero failures/errors/skips, zero missed production lines/branches, and warning-free public Javadocs.
  7. Keep Draft. Report the exact new head, parent comparison, changed files, deterministic interleaving evidence, and any migration/compatibility consequence. Request fresh CodeRabbit/OpenCode/Noema review only after the new exact head exists.

Do not dismiss the finding merely because #263 will later add a durable outbox; this PR must not ship a tenant-crossing retry path.

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@opencode-agent @cwl-noema-review Exact-head update: independently inspect d55490707c24a71ae26ef208e0c0f6618606275c relative to parent #270 26563218ae42eaa876c784fcf56b27f8cb810080; all predecessor-head review and check evidence is stale.

Verify the restored parent acceptance gates in AGENTS.md, the test-first prohibition on raw administrative job UUIDs, actor/tenant/job HMAC domain separation, absent/unavailable behavior, captured-log exclusions, full public Javadocs, the authoritative security decision, and the reconciled CHANGELOG.md. Run mvn -B --no-transfer-progress verify and python -m pytest -q scripts on this exact head and report test totals, zero failures/errors/skips, zero missed production lines/branches, and Javadoc warnings. Exact-head fuzz 31067988197 is pending; CI, Security Scan, and SAST are absent on this stacked-base PR and must not be inferred. Keep Draft and do not merge or infer independent approval.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='d55490707c24a71ae26ef208e0c0f6618606275c'
parent='26563218ae42eaa876c784fcf56b27f8cb810080'

echo '== Exact relationship =='
git show -s --format='HEAD %H%nParents %P%nSubject %s' "$head"
printf 'merge_base='; git merge-base "$head" "$parent"
printf 'ahead_behind='; git rev-list --left-right --count "$parent...$head"

echo '== Changed paths =='
git diff --name-status "$parent" "$head"
printf 'changed_file_count='
git diff --name-only "$parent" "$head" | awk 'NF {n++} END {print n+0}'

echo '== Relevant parent-relative diffs =='
git diff --unified=20 "$parent" "$head" -- \
  AGENTS.md CHANGELOG.md \
  ':(glob)docs/security/*administrative*' \
  ':(glob)**/AdministrativeAuditLogger.java' \
  ':(glob)**/AdministrativeAuditLoggerTest.java' \
  ':(glob)**/AuditPseudonymizer.java' \
  ':(glob)**/AuditPseudonymizerTest.java' \
  ':(glob)**/InMemoryConversionJobRepository.java' \
  ':(glob)**/InMemoryConversionJobRepository*Test.java' \
  ':(glob)**/DefaultDocumentConversionService.java' \
  ':(glob)**/DefaultConversionWorker.java'

echo '== Relevant file outlines =='
fd -t f -a . src/main/java src/test/java | rg \
  '(AdministrativeAuditLogger|AuditPseudonymizer|InMemoryConversionJobRepository|DefaultDocumentConversionService|DefaultConversionWorker)' \
  | sort | while IFS= read -r f; do
    echo "--- $f ---"
    ast-grep outline "$f" --items all --view compact 2>/dev/null || true
  done

echo '== Audit source, call sites, raw identifier fields, and HMAC domains =='
rg -n -i -C 4 \
  'jobFingerprint|jobId=|forAdministrative(Job|Actor|Tenant)|administrative.*(job|actor|tenant)|unavailable|absent|logger\.(info|warn|error|debug|trace)' \
  src/main/java src/test/java docs/security 2>/dev/null || true

echo '== Retry/replacement/worker paths and deterministic tests =='
rg -n -C 6 \
  'retryDeadLetteredForTenant|jobIndexLock|computeIfPresent|save\s*\(|enqueue\s*\(|claimForProcessing|findById\s*\(|same.?UUID|replacement|generation|collision' \
  src/main/java src/test/java 2>/dev/null || true

echo '== Acceptance-gate and Javadoc checks in AGENTS.md =='
rg -n -i -C 3 \
  'mvn -B --no-transfer-progress verify|zero.*skip|exact.head|Netty|JaCoCo|missed.*(line|branch)|Javadoc|warning|CLEARFOLIO_SECRET_CONFIG_DIR|config.tree' \
  AGENTS.md

echo '== Existing Maven/Python report artifacts, if versioned or present =='
fd -H -t f . | rg \
  '(surefire-reports|failsafe-reports|jacoco|site/apidocs|javadoc|pytest|test-results|\.xml$)' \
  | head -200 || true

Length of output: 203044


🏁 Script executed:

#!/bin/bash
set -euo pipefail

show() {
  local f="$1" a="$2" b="$3"
  echo "===== $f lines $a-$b ====="
  nl -ba "$f" | sed -n "${a},${b}p"
}

echo '== Exact head commits after the prior reviewed head =='
git log --format='%H %s' --ancestry-path \
  fe2e53328a0b44c1c449d724314c6c4944eb572b..d55490707c24a71ae26ef208e0c0f6618606275c

echo '== Parent-relative changed paths only =='
git diff --name-only 26563218ae42eaa876c784fcf56b27f8cb810080 d55490707c24a71ae26ef208e0c0f6618606275c

show src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java 1 390
show src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryTenantScopeTest.java 55 230
show src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryConcurrencyTest.java 1 360

echo '== Tests added or modified after the previous review head =='
git diff --name-only fe2e53328a0b44c1c449d724314c6c4944eb572b d55490707c24a71ae26ef208e0c0f6618606275c \
  | rg 'src/test|InMemoryConversionJobRepository|DefaultConversionWorker|DefaultDocumentConversionService' || true
git diff --unified=30 fe2e53328a0b44c1c449d724314c6c4944eb572b d55490707c24a71ae26ef208e0c0f6618606275c -- \
  ':(glob)**/InMemoryConversionJobRepository*.java' \
  ':(glob)**/DefaultDocumentConversionService*.java' \
  ':(glob)**/DefaultConversionWorker*.java'

show src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java 1 260
show src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java 1 300
show src/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java 1 340
show src/test/java/com/clearfolio/viewer/security/AuditPseudonymizerTest.java 1 340
show docs/security/2026-08-05-administrative-authorization.md 1 360

echo '== Changelog administrative-security entries =='
rg -n -i -C 5 'administrative|tenant.claim|audit|pseudonym|conversion.job|job fingerprint' CHANGELOG.md

echo '== Current report evidence content and commit provenance =='
for f in \
  docs/qa/evidence/2026-07-02-krw2b-sale-readiness/jacoco-status.txt \
  docs/qa/evidence/2026-07-02-krw2b-sale-readiness/javadoc-status.txt \
  docs/qa/evidence/2026-02-21-ac-gates/jacoco.csv
do
  echo "--- $f ---"
  sed -n '1,160p' "$f"
  echo "--- last commit touching $f ---"
  git log -1 --format='%H %ad %s' --date=short -- "$f"
done

Length of output: 5831


🏁 Script executed:

#!/bin/bash
set -euo pipefail

show() {
  local f="$1" a="$2" b="$3"
  echo "===== $f lines $a-$b ====="
  awk -v start="$a" -v end="$b" 'NR >= start && NR <= end { printf "%6d  %s\n", NR, $0 }' "$f"
}

show src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java 1 390
show src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryTenantScopeTest.java 55 230
show src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryConcurrencyTest.java 1 360

echo '== Tests modified since the prior reviewed head =='
git diff --name-only fe2e53328a0b44c1c449d724314c6c4944eb572b d55490707c24a71ae26ef208e0c0f6618606275c \
  | rg 'src/test|InMemoryConversionJobRepository|DefaultConversionWorker|DefaultDocumentConversionService' || true

show src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java 1 260
show src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java 1 300
show src/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java 1 340
show src/test/java/com/clearfolio/viewer/security/AuditPseudonymizerTest.java 1 340
show docs/security/2026-08-05-administrative-authorization.md 1 360

echo '== Changelog administrative-security entries =='
rg -n -i -C 5 'administrative|tenant.claim|audit|pseudonym|conversion.job|job fingerprint' CHANGELOG.md

echo '== Existing report evidence and provenance =='
for f in \
  docs/qa/evidence/2026-07-02-krw2b-sale-readiness/jacoco-status.txt \
  docs/qa/evidence/2026-07-02-krw2b-sale-readiness/javadoc-status.txt \
  docs/qa/evidence/2026-02-21-ac-gates/jacoco.csv
do
  echo "--- $f ---"
  sed -n '1,160p' "$f"
  echo "--- last commit touching $f ---"
  git log -1 --format='%H %ad %s' --date=short -- "$f"
done

Length of output: 89751


@seonghobae I inspected exact head d55490707c24a71ae26ef208e0c0f6618606275c. I treated predecessor evidence as stale.

I verified these items:

  • AGENTS.md restores the authoritative mvn -B --no-transfer-progress verify gate.
  • AGENTS.md requires zero failures, errors, and skips.
  • AGENTS.md requires zero missed production lines and branches.
  • AGENTS.md requires warning-free public Javadocs.
  • AGENTS.md preserves the Netty and exact-head requirements.
  • AGENTS.md retains config-tree loading through CLEARFOLIO_SECRET_CONFIG_DIR.
  • AdministrativeAuditLogger emits jobFingerprint, not jobId=.
  • AuditPseudonymizer uses separate administrative actor, tenant, and job HMAC domains.
  • The captured-log test rejects both the raw UUID and jobId=.
  • The audit code uses explicit absent:<version> and unavailable:<version> markers.
  • The security decision and CHANGELOG.md document the administrative audit fingerprint contract.

I found one remaining actionable issue.

[P1] Reject live UUID replacement to close the retry dispatch race
InMemoryConversionJobRepository.save still replaces a live job with jobs.put under jobIndexLock. retryDeadLetteredForTenant still updates a tenant-owned job outside that lock. After it returns ACCEPTED, DefaultDocumentConversionService enqueues only jobId. DefaultConversionWorker then resolves that identifier with unscoped findById.

A concurrent caller can replace Tenant A's accepted retry job with Tenant B's distinct job before worker processing. The worker can then claim and process Tenant B's replacement. The existing tests explicitly accept this live same-UUID replacement. The concurrency test covers delete versus replacement only. No retry-versus-replacement regression was added after the prior reviewed head.

Make save fail closed when a distinct live object already owns the UUID. Keep an explicit and tested deletion or tombstone policy if identifier reuse is required. Add a deterministic test that pauses retry dispatch, attempts a cross-tenant same-UUID replacement, and verifies that the replacement is neither enqueued nor processed.

Affected code: src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java, Line 40; src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java, Line 305; src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java, Line 261; src/main/java/com/clearfolio/viewer/service/DefaultConversionWorker.java, Line 196.

I could not run mvn -B --no-transfer-progress verify or python -m pytest -q scripts because this sandbox cannot execute repository compilers or interpreters. Therefore, exact-head test totals, zero failures/errors/skips, zero missed production lines/branches, and Javadoc warnings remain unverified. The checked-in JaCoCo and Javadoc artifacts are from older commits and are not exact-head evidence.

Exact-head fuzz 31067988197 remains pending. CI, Security Scan, and SAST remain absent. I did not treat them as passing. I did not approve or merge this draft.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (6)
src/test/java/com/clearfolio/viewer/controller/AdminControllerTenantMutationBoundaryTest.java (1)

39-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

두 테스트 파일의 CLAIM_SECRET이 32바이트 경계에 정확히 걸쳐 있습니다. "tenant-claims-" + "integration-secret"는 정확히 32바이트입니다. TenantAccessService.requireSigned는 32바이트 미만의 secret을 503으로 거부합니다. 두 리터럴 중 하나라도 짧아지면 해당 파일의 모든 테스트가 기대한 상태 코드 대신 503을 받고, 실패 원인이 드러나지 않습니다.

  • src/test/java/com/clearfolio/viewer/controller/AdminControllerTenantMutationBoundaryTest.java#L39-L40: CLAIM_SECRET을 바로 아래 AUDIT_SECRET과 같이 길이가 명시적인 "0123456789abcdef".repeat(2) 형태로 바꾸십시오.
  • src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java#L35-L36: 같은 방식으로 CLAIM_SECRET을 길이가 명시적인 리터럴로 바꾸십시오.
🤖 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
`@src/test/java/com/clearfolio/viewer/controller/AdminControllerTenantMutationBoundaryTest.java`
around lines 39 - 40, Replace CLAIM_SECRET in
src/test/java/com/clearfolio/viewer/controller/AdminControllerTenantMutationBoundaryTest.java
lines 39-40 and
src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java lines
35-36 with an explicitly sized 32-byte literal matching the adjacent
AUDIT_SECRET pattern, while leaving AUDIT_SECRET unchanged.
src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java (1)

195-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

중복된 실패 케이스를 하나로 합치십시오.

lookupFailureIddeleteFailureId는 동일하게 스텁되고 동일하게 검증됩니다. 두 경우 모두 deleteJob(UUID, TenantContext)IllegalStateException을 던지고 5xx를 기대합니다. 컨트롤러는 이제 단일 테넌트 범위 호출만 수행하므로 별도의 lookup 단계가 없습니다. 두 이름은 존재하지 않는 단계를 암시합니다.

실패 케이스 하나만 남기십시오.

🤖 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 `@src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java`
around lines 195 - 226, In
deleteUsesTenantScopedServiceAndReportsFailuresGenerically, remove the redundant
lookupFailureId scenario and retain a single delete failure case. Update the
stubbing, request assertion, and tenant-scoped verification so only the
remaining failure identifier is exercised, while preserving the success and
generic 5xx expectations.
src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryConcurrencyTest.java (2)

123-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

이 테스트는 잠금 구현 방식에 결합되어 있습니다.

assertBlockedByDeleteCriticalSectionThread.State.BLOCKEDThreadInfo.getLockOwnerId()를 사용합니다. 두 값은 내장 모니터(synchronized)에만 적용됩니다. InMemoryConversionJobRepositoryjobIndexLockReentrantLock으로 바꾸면 대기 스레드는 WAITING 상태가 되고 getLockOwnerId()-1을 반환합니다. 그러면 결함이 없어도 테스트가 실패합니다.

관측 가능한 결과, 즉 삭제가 임계 구역을 해제하기 전에는 교체가 인덱스 작업에 도달하지 않는다는 점만 검증하는 방식을 고려하십시오. 현재 이 테스트에는 이미 replacementIndexReached 래치로 그 검증이 있습니다.

🤖 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
`@src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryConcurrencyTest.java`
around lines 123 - 145, Update assertBlockedByDeleteCriticalSection to stop
inspecting Thread.State.BLOCKED and ThreadInfo.getLockOwnerId(), which couples
the test to intrinsic monitor locking. Rely on the existing
replacementIndexReached latch to assert that replacement indexing has not been
reached while delete cleanup remains held, preserving verification of the
observable synchronization behavior without assuming a specific lock
implementation.

86-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

JUnit 어서션 관용구를 사용하십시오.

현재 어서션은 실패 시 정보가 부족합니다. assertTrue(false, msg)fail(msg)로, assertTrue(!x, msg)assertFalse(x, msg)로, assertTrue(obj != null, msg)assertNotNull(obj, msg)로, assertTrue(a == b, msg)assertEquals(b, a, msg)로 바꾸십시오. assertEquals는 기대값과 실제값을 함께 출력하므로 잠금 소유자 불일치를 진단하기 쉽습니다.

♻️ 제안 변경
                 assertTrue(
                         replacementTaskStarted.await(2, TimeUnit.SECONDS),
                         "replacement save task did not start"
                 );
                 assertBlockedByDeleteCriticalSection(
                         replacementThread.get(),
                         deleteThread.get(),
                         saveResult
                 );
-                assertTrue(
-                        replacementIndexReached.getCount() == 1L,
-                        "replacement reached index work before delete released the critical section"
-                );
+                assertEquals(
+                        1L,
+                        replacementIndexReached.getCount(),
+                        "replacement reached index work before delete released the critical section"
+                );
         long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(2);
         while (System.nanoTime() < deadline) {
-            assertTrue(
-                    !saveResult.isDone(),
-                    "replacement save completed before delete cleanup was released"
-            );
+            assertFalse(
+                    saveResult.isDone(),
+                    "replacement save completed before delete cleanup was released"
+            );
             if (replacementThread.getState() == Thread.State.BLOCKED) {
                 ThreadInfo threadInfo = ManagementFactory.getThreadMXBean().getThreadInfo(
                         replacementThread.threadId()
                 );
-                assertTrue(threadInfo != null, "replacement thread metadata was unavailable");
-                assertTrue(
-                        threadInfo.getLockOwnerId() == deleteThread.threadId(),
-                        "replacement save blocked on a monitor not owned by the delete operation"
-                );
+                assertNotNull(threadInfo, "replacement thread metadata was unavailable");
+                assertEquals(
+                        deleteThread.threadId(),
+                        threadInfo.getLockOwnerId(),
+                        "replacement save blocked on a monitor not owned by the delete operation"
+                );
                 return;
             }
             Thread.onSpinWait();
         }
-        assertTrue(
-                false,
-                "replacement save did not block on the delete critical section; observed state="
-                        + replacementThread.getState()
-        );
+        fail(
+                "replacement save did not block on the delete critical section; observed state="
+                        + replacementThread.getState()
+        );

assertEquals, assertFalse, assertNotNull, fail의 정적 임포트를 추가하십시오.

Also applies to: 130-151

🤖 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
`@src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryConcurrencyTest.java`
around lines 86 - 89, Update the assertions in
InMemoryConversionJobRepositoryConcurrencyTest, including the
replacementIndexReached check and the assertions in the additional highlighted
range, to use JUnit idioms: convert equality checks to assertEquals with
expected and actual values, negated checks to assertFalse, null checks to
assertNotNull, and unconditional failures to fail. Add the corresponding static
imports for assertEquals, assertFalse, assertNotNull, and fail, while preserving
each existing failure message.
src/test/java/com/clearfolio/viewer/repository/ConversionJobRepositoryTest.java (1)

58-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

인터페이스 기본 구현 두 개가 이 테스트에서 검증되지 않습니다.

이 테스트는 findByTenantAndIdfindByTenantAndContentHash의 기본 구현만 실행합니다. findAllByTenantIddeleteByTenantAndId의 기본 구현은 익명 어댑터로 실행되지 않습니다. 두 기본 구현이 각각 List.of()false를 반환하고 findAll(), deleteById()를 호출하지 않는지 같은 방식으로 검증하십시오.

프로덕션 변경은 com.clearfolio.viewer.*에 대해 100% JaCoCo 라인 및 브랜치 커버리지를 유지해야 합니다.

💚 기본 구현 검증 추가
         assertTrue(repository.findByTenantAndContentHash("tenant-a", null).isEmpty());
+        assertTrue(repository.findAllByTenantId("tenant-a").isEmpty());
+        assertTrue(repository.findAllByTenantId(null).isEmpty());
+        assertFalse(repository.deleteByTenantAndId("tenant-a", jobId));
+        assertFalse(repository.deleteByTenantAndId(null, jobId));
         assertEquals(0, globalIdentifierLookupCalls.get());
         assertEquals(0, globalContentLookupCalls.get());

이 익명 어댑터의 findAll()List.of()를 반환하고 deleteById()는 아무 동작도 하지 않습니다. 전역 경로 사용을 확실히 배제하려면 두 메서드도 카운터 증가 후 AssertionError를 던지도록 변경하십시오.

As per coding guidelines: "Every production change must maintain 100% JaCoCo line and branch coverage for com.clearfolio.viewer.*."

🤖 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
`@src/test/java/com/clearfolio/viewer/repository/ConversionJobRepositoryTest.java`
around lines 58 - 67, Extend the anonymous adapter tests around
findByTenantAndId and findByTenantAndContentHash to invoke findAllByTenantId and
deleteByTenantAndId, asserting the former returns List.of(), the latter returns
false, and both global lookup counters remain zero. Update the adapter’s
findAll() and deleteById() implementations to increment their respective
counters before throwing AssertionError, ensuring the default paths are
exercised without allowing global operations.

Source: Coding guidelines

src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryTenantScopeTest.java (1)

137-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

이 테스트들은 살아 있는 UUID 교체를 허용된 동작으로 고정합니다.

두 테스트 모두 repository.save(replacementSouth)tenant-north가 소유한 동일 UUID 작업을 교체하도록 허용한 뒤, 오래된 관찰에 근거한 삭제와 재시도가 차단되는지 검증합니다. 삭제와 재시도 경계는 올바릅니다.

그러나 두 테스트는 교체 자체를 유효한 동작으로 고정합니다. InMemoryConversionJobRepository.java 라인 40-47에 남긴 지적과 같은 근본 원인입니다. 그 지적을 해결하기 위해 save가 서로 다른 살아 있는 동일 UUID 교체를 거부하도록 바꾸면, 이 두 테스트의 준비 단계도 함께 수정해야 합니다.

Also applies to: 192-222

🤖 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
`@src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryTenantScopeTest.java`
around lines 137 - 158, Update the setup in both tests,
staleTenantObservationCannotDeleteAReplacementOwnedByAnotherTenant and the
additional test covering the same replacement scenario, to reflect that save
rejects replacing an existing live job with the same UUID under a different
tenant. Do not rely on repository.save(replacementSouth) to create the
replacement; establish the replacement through the supported test setup or
assert the expected save rejection before exercising the deletion/retry
behavior, while preserving the existing boundary assertions.
🤖 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 `@docs/security/2026-08-05-administrative-authorization.md`:
- Around line 42-44: The retry dispatch flow after retryDeadLetteredForTenant
must bind queued work to the authorized tenant and immutable generation, not
only the UUID. Update conversionWorker.enqueue and DefaultConversionWorker
execution-time lookup to carry and revalidate both values, rejecting
replacements that do not match; add a deterministic CountDownLatch regression
test reproducing same-UUID replacement races. Update
docs/security/2026-08-05-administrative-authorization.md lines 42-44 and
CHANGELOG.md lines 41-42 only after implementation and tests are complete, so
they do not claim the protection is finished prematurely.

In `@src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java`:
- Around line 47-78: Update the JavaDoc for forAdministrativeActor,
forAdministrativeTenant, and forAdministrativeJob to document that secret must
contain at least 32 UTF-8 bytes, describe the behavior for an empty secret, and
state the conditions under which keyVersion validation throws
IllegalArgumentException. Keep the implementation unchanged while making each
public factory’s input and exception contract explicit.

In
`@src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java`:
- Around line 330-336: Update deleteArtifact in DefaultDocumentConversionService
to record failed artifact deletions for the existing retry queue or cleanup
workflow instead of only logging and swallowing the exception. Ensure
artifactStore.deletePdf failures are also counted by the appropriate failure
metric, while preserving successful deletion behavior.

In `@src/main/resources/application-buyer-demo.yml`:
- Around line 28-30: Update the clearfolio.artifact-token.secret configuration
in application-buyer-demo.yml to read from the KV-backed config-tree hmac-secret
entry instead of CLEARFOLIO_ARTIFACT_TOKEN_SECRET, and remove the corresponding
export from the deployment documentation.

In
`@src/test/java/com/clearfolio/viewer/config/BuyerDemoSecretConfigurationTest.java`:
- Around line 22-23: Update BuyerDemoSecretConfigurationTest to reject any
hmac-secret definition, not only interpolated values: assert that
application-buyer-demo.yml contains no hmac-secret: key at all, while retaining
the existing tenant-claims secret check.

In
`@src/test/java/com/clearfolio/viewer/service/TenantScopedRetryContractTest.java`:
- Around line 121-148: 테넌트 소유 작업의 재시도가 ACCEPTED 된 뒤 워커가 UUID로 조회하기 전에 동일 UUID의
작업 교체가 발생하는 인터리빙을 검증하는 결정론적 회귀 테스트를 추가하십시오.
InMemoryConversionJobRepositoryConcurrencyTest의 삭제 경로에서 사용하는 래치 패턴을 재시도 경로에
적용하고, 워커가 조회한 작업이 재시도를 승인한 테넌트의 작업인지 확인하십시오.

---

Nitpick comments:
In
`@src/test/java/com/clearfolio/viewer/controller/AdminControllerTenantMutationBoundaryTest.java`:
- Around line 39-40: Replace CLAIM_SECRET in
src/test/java/com/clearfolio/viewer/controller/AdminControllerTenantMutationBoundaryTest.java
lines 39-40 and
src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java lines
35-36 with an explicitly sized 32-byte literal matching the adjacent
AUDIT_SECRET pattern, while leaving AUDIT_SECRET unchanged.

In `@src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java`:
- Around line 195-226: In
deleteUsesTenantScopedServiceAndReportsFailuresGenerically, remove the redundant
lookupFailureId scenario and retain a single delete failure case. Update the
stubbing, request assertion, and tenant-scoped verification so only the
remaining failure identifier is exercised, while preserving the success and
generic 5xx expectations.

In
`@src/test/java/com/clearfolio/viewer/repository/ConversionJobRepositoryTest.java`:
- Around line 58-67: Extend the anonymous adapter tests around findByTenantAndId
and findByTenantAndContentHash to invoke findAllByTenantId and
deleteByTenantAndId, asserting the former returns List.of(), the latter returns
false, and both global lookup counters remain zero. Update the adapter’s
findAll() and deleteById() implementations to increment their respective
counters before throwing AssertionError, ensuring the default paths are
exercised without allowing global operations.

In
`@src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryConcurrencyTest.java`:
- Around line 123-145: Update assertBlockedByDeleteCriticalSection to stop
inspecting Thread.State.BLOCKED and ThreadInfo.getLockOwnerId(), which couples
the test to intrinsic monitor locking. Rely on the existing
replacementIndexReached latch to assert that replacement indexing has not been
reached while delete cleanup remains held, preserving verification of the
observable synchronization behavior without assuming a specific lock
implementation.
- Around line 86-89: Update the assertions in
InMemoryConversionJobRepositoryConcurrencyTest, including the
replacementIndexReached check and the assertions in the additional highlighted
range, to use JUnit idioms: convert equality checks to assertEquals with
expected and actual values, negated checks to assertFalse, null checks to
assertNotNull, and unconditional failures to fail. Add the corresponding static
imports for assertEquals, assertFalse, assertNotNull, and fail, while preserving
each existing failure message.

In
`@src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryTenantScopeTest.java`:
- Around line 137-158: Update the setup in both tests,
staleTenantObservationCannotDeleteAReplacementOwnedByAnotherTenant and the
additional test covering the same replacement scenario, to reflect that save
rejects replacing an existing live job with the same UUID under a different
tenant. Do not rely on repository.save(replacementSouth) to create the
replacement; establish the replacement through the supported test setup or
assert the expected save rejection before exercising the deletion/retry
behavior, while preserving the existing boundary assertions.
🪄 Autofix

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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8ad227da-443e-45cb-a27f-9b6d4ba4a0f4

📥 Commits

Reviewing files that changed from the base of the PR and between 2656321 and d554907.

📒 Files selected for processing (31)
  • AGENTS.md
  • CHANGELOG.md
  • docs/deployment/2026-07-02-buyer-deployment-integration-playbook.md
  • docs/deployment/clearfolio-buyer-connector.openapi.yaml
  • docs/security/2026-08-05-administrative-authorization.md
  • src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java
  • src/main/java/com/clearfolio/viewer/auth/TenantAccessService.java
  • src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java
  • src/main/java/com/clearfolio/viewer/controller/AdminController.java
  • src/main/java/com/clearfolio/viewer/repository/ConversionJobRepository.java
  • src/main/java/com/clearfolio/viewer/repository/ConversionJobStateStore.java
  • src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java
  • src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java
  • src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java
  • src/main/java/com/clearfolio/viewer/service/DocumentConversionService.java
  • src/main/resources/application-buyer-demo.yml
  • src/test/java/com/clearfolio/viewer/audit/AdministrativeAuditLoggerTest.java
  • src/test/java/com/clearfolio/viewer/auth/TenantAccessServiceStrictClaimsTest.java
  • src/test/java/com/clearfolio/viewer/config/BuyerDemoSecretConfigurationTest.java
  • src/test/java/com/clearfolio/viewer/controller/AdminControllerSignedClaimsRequirementTest.java
  • src/test/java/com/clearfolio/viewer/controller/AdminControllerTenantMutationBoundaryTest.java
  • src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java
  • src/test/java/com/clearfolio/viewer/repository/ConversionJobRepositoryTest.java
  • src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryConcurrencyTest.java
  • src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryMissingIdentifierTest.java
  • src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryTenantScopeTest.java
  • src/test/java/com/clearfolio/viewer/security/AuditPseudonymizerTest.java
  • src/test/java/com/clearfolio/viewer/service/TenantScopedAdministrativeListContractTest.java
  • src/test/java/com/clearfolio/viewer/service/TenantScopedAtomicMutationBoundaryTest.java
  • src/test/java/com/clearfolio/viewer/service/TenantScopedRetryContractTest.java
  • src/test/java/com/clearfolio/viewer/service/TenantScopedServiceDefaultsTest.java

Comment thread docs/security/2026-08-05-administrative-authorization.md Outdated
Comment thread src/main/resources/application-buyer-demo.yml Outdated
Comment thread src/test/java/com/clearfolio/viewer/config/BuyerDemoSecretConfigurationTest.java Outdated

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Continue from exact current head 65dbb359397b9014ab3e02380650633f35abdfde and close the immutable-identifier repair without broadening the stack.

The P1 race is now addressed in source by permanently reserving accepted UUIDs, rejecting distinct live/tombstoned rebinding before secondary-index work, defining the durable repository contract, and adding deterministic retry/delete/find-or-store concurrency regressions. Review the resulting implementation rather than reapplying a competing rewrite.

Required completion:

  1. Run the focused repository and tenant-retry tests, then mvn -B --no-transfer-progress verify and python -m pytest -q scripts. Preserve zero failures/errors/skips, zero missed production lines/branches, and warning-free public Javadocs.
  2. Confirm save idempotence is limited to the exact same live object; distinct live and tombstoned identifiers fail closed through both save and findOrStoreByContentHash.
  3. Confirm deletion removes the live record and tenant/hash index but retains identifier reservation; the deterministic retry boundary must enqueue only the original Tenant A object and produce no Tenant B lifecycle evidence.
  4. Correct only the stale administrative ADR and CHANGELOG.md statements that still describe same-UUID replacement as supported. They must instead state immutable/non-reusable lifecycle identity and tombstone/generation requirements for durable adapters.
  5. Inspect exact-head coverage for every new branch (existing == job, live collision, tombstoned collision, indexed/unindexed save). Add only focused tests proven necessary by the report.
  6. Do not add a workflow, dependency, global lookup fallback, temporary script, release, or unrelated refactor. Keep Draft.
  7. Report the exact final head, parent comparison, bounded file list, test totals, coverage/Javadoc evidence, and no temporary/write-scoped workflow. Request fresh CodeRabbit/OpenCode/Noema review only after the verified head exists.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Stop broadening this security stack at exact current head 88f5c9b0ed2b2f90f95041834d642355218eb5b2.

The new ArtifactCleanupQueue, ArtifactCleanupQueueTest, and global @EnableScheduling are not part of the bounded administrative-authorization repair requested here. They introduce a separate process-local lifecycle subsystem owned by issue #263, expand the parent-relative slice from 31 to 35 files, and are not wired into the administrative delete service in the current head. In addition, retryPending logs the caught exception object; FileSystemArtifactStore.deletePdf embeds the raw document UUID in that exception message, contradicting the queue's stated no-raw-identifier logging boundary.

Required correction:

  1. Remove src/main/java/com/clearfolio/viewer/artifact/ArtifactCleanupQueue.java and its test from this branch.
  2. Restore ClearfolioViewerApplication exactly to parent fix(security): harden audit pseudonymization and refresh Netty evidence #270 with no global @EnableScheduling change.
  3. Keep issue [Product Gap] Complete tenant-safe document lifecycle and artifact deletion #263 as the dedicated durable deletion receipt/outbox/cleanup-worker product slice; do not add a process-local substitute here.
  4. Retain the immutable/non-reusable job-identifier fix, repository contract, deterministic retry/delete collision tests, config-tree corrections, and updated administrative ADR.
  5. Update the two stale CHANGELOG.md security bullets so they describe rejection of distinct live/tombstoned UUID rebinding and durable tombstone/generation requirements—not supported same-UUID replacement.
  6. Run the focused repository/retry tests and full mvn -B --no-transfer-progress verify; preserve zero failures/errors/skips, zero missed production lines/branches, and warning-free public Javadocs.
  7. Keep Draft and report a final exact head whose parent-relative scope contains only the authorization and identifier-ownership changes. Do not add another lifecycle component, workflow, dependency, scheduler, release, or unrelated test.

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@opencode-agent @cwl-noema-review Review exact current head 73d1f77c67e417c51aa7548b7a43d4aea6ad4463 relative to parent #270 exact 26563218ae42eaa876c784fcf56b27f8cb810080. This is the bounded 32-file administrative authorization and immutable job-identity slice; process-local cleanup queue/scheduler files have been removed and issue #263 remains the durable deletion-lifecycle owner.

Verify signed-claim fail-closed construction, admin:read/admin:write, tenant-scoped list/delete/retry boundaries, missing/null/cross-tenant concealment, privacy-safe actor/tenant/job HMAC domains, config-tree secret sourcing, fail-closed adapter defaults, tenant-bound content indexes, permanent UUID reservation, exact-object-only save idempotence, live/tombstoned collision rejection before index work, deterministic delete/save and retry/save interleavings, worker enqueue resolving only the authorized original object, and durable adapter tombstone/generation requirements. Verify no cleanup queue, global scheduling enablement, temporary workflow, global lookup fallback, raw tenant/job/token logging, dependency drift, release, or unrelated automation remains.

Run or inspect focused tests plus mvn -B --no-transfer-progress verify and python -m pytest -q scripts; preserve zero failures/errors/skips, zero missed production lines/branches, and warning-free public Javadocs. Report exact-head actionable findings or no-blocker advisory evidence only. Keep Draft and do not infer counted independent approval.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Correct the authoritative evidence at exact current head 8713d204e6f38b27d8fd00baf7c1c1e6acd13873 without broadening this administrative-authorization slice.

The process-local ArtifactCleanupQueue implementation and scheduler were removed, but three authoritative documents still claim they exist. Production DefaultDocumentConversionService.deleteArtifact still logs and swallows artifact-store deletion failures; therefore the branch cannot claim deduplicated pending work, retry cadence, aggregate cleanup metrics, cleanup-aware store decoration, or associated tests.

Required bounded correction:

  1. In AGENTS.md, remove the Durable cleanup boundary section that mandates a configured ArtifactCleanupQueue. Replace it only with an honest gap statement: issue [Product Gap] Complete tenant-safe document lifecycle and artifact deletion #263 owns restart-safe deletion receipts/outbox/cleanup worker; this PR does not implement that subsystem.
  2. In docs/deployment/2026-07-02-buyer-deployment-integration-playbook.md, remove every claim that failed deletion is retained in a process-local queue, that the artifact store is wrapped by ArtifactCleanupQueue, that a retry cadence exists, or that deletion failures produce aggregate evidence. State instead that owned repository deletion precedes best-effort artifact removal, failure can leave orphaned bytes, and production cutover remains blocked on [Product Gap] Complete tenant-safe document lifecycle and artifact deletion #263 durable cleanup/outbox evidence.
  3. In docs/security/2026-08-05-administrative-authorization.md, remove the same nonexistent queue, metrics, evidence, and verification claims. Preserve the valid tenant-authorization ordering and explicitly record failed artifact deletion durability as an unresolved [Product Gap] Complete tenant-safe document lifecycle and artifact deletion #263 boundary.
  4. Keep the valid immutable/non-reusable conversion-job UUID contract and the corrected CHANGELOG.md lifecycle bullets. Do not add a scheduler, process-local queue, dependency, workflow, release change, or new production subsystem.
  5. Add or update a deterministic documentation-contract test only if one already governs these authoritative claims; otherwise keep this documentation-only correction minimal.
  6. Run the authoritative mvn -B --no-transfer-progress verify and preserve zero failures/errors/skips, zero missed production lines/branches, and warning-free public Javadocs. Report the exact resulting head and do not claim absent CI/Security/SAST evidence.

@seonghobae
seonghobae force-pushed the fix/admin-endpoint-auth-clean branch from 8713d20 to 73d1f77 Compare August 6, 2026 04:07
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='73d1f77c67e417c51aa7548b7a43d4aea6ad4463'
parent='26563218ae42eaa876c784fcf56b27f8cb810080'

echo '== Exact relationship =='
git show -s --format='HEAD %H%nParents %P%nSubject %s' "$head"
printf 'merge_base='; git merge-base "$head" "$parent"
printf 'ahead_behind='; git rev-list --left-right --count "$parent...$head"

echo '== Parent-relative paths =='
git diff --name-status "$parent" "$head"
printf 'changed_file_count='
git diff --name-only "$parent" "$head" | awk 'NF { n++ } END { print n+0 }'

echo '== Prohibited cleanup/scheduling/workflow paths and annotations =='
git diff --name-only "$parent" "$head" | rg -i \
  '(ArtifactCleanupQueue|ClearfolioViewerApplication|\.github/workflows|workflow|scheduler|scheduling)' || true
rg -n -C2 '`@EnableScheduling`|ArtifactCleanupQueue|retryPending' \
  src/main/java src/test/java 2>/dev/null || true

echo '== Primary implementation outlines =='
for f in \
  src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java \
  src/main/java/com/clearfolio/viewer/repository/ConversionJobRepository.java \
  src/main/java/com/clearfolio/viewer/repository/ConversionJobStateStore.java \
  src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java \
  src/main/java/com/clearfolio/viewer/service/DefaultConversionWorker.java \
  src/main/java/com/clearfolio/viewer/auth/TenantAccessService.java \
  src/main/java/com/clearfolio/viewer/controller/AdminController.java \
  src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java \
  src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java
do
  echo "--- $f ---"
  ast-grep outline "$f" --items all --view compact 2>/dev/null || true
done

echo '== UUID reservation, retry, dispatch, tenant/default paths =='
rg -n -C5 \
  'reserved|reservation|tombstone|generation|existing == job|jobs\.put|putIfAbsent|findOrStoreByContentHash|retryDeadLetteredForTenant|deleteByTenantAndId|jobIndexLock|enqueue\s*\(|claimForProcessing|findById\s*\(|findAllByTenantId|findByTenantAnd(Id|ContentHash)' \
  src/main/java/com/clearfolio/viewer \
  -g '*.java'

echo '== Deterministic ownership/concurrency tests =='
rg -n -i -C5 \
  'retry.*save|save.*retry|same.?UUID|tombston|reservation|replacement|collision|interleav|latch|enqueue|lifecycle' \
  src/test/java/com/clearfolio/viewer \
  -g '*.java'

echo '== Audit, claims, permissions, config-tree, and raw logging terms =='
rg -n -i -C3 \
  'requireSigned|admin:(read|write)|forAdministrative(Job|Actor|Tenant)|jobFingerprint|jobId=|CLEARFOLIO_SECRET_CONFIG_DIR|config.tree|X-Clearfolio|logger\.(info|warn|error|debug|trace)' \
  src/main/java src/main/resources src/test/java docs AGENTS.md \
  -g '*.java' -g '*.yml' -g '*.yaml' -g '*.md'

Length of output: 203318


🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='73d1f77c67e417c51aa7548b7a43d4aea6ad4463'
parent='26563218ae42eaa876c784fcf56b27f8cb810080'

show() {
  local file="$1" start="$2" end="$3"
  echo "===== ${file} lines ${start}-${end} ====="
  nl -ba "$file" | sed -n "${start},${end}p"
}

show src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java 1 390
show src/main/java/com/clearfolio/viewer/repository/ConversionJobRepository.java 1 230
show src/main/java/com/clearfolio/viewer/repository/ConversionJobStateStore.java 1 180
show src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java 220 315
show src/main/java/com/clearfolio/viewer/service/DefaultConversionWorker.java 160 230
show src/main/java/com/clearfolio/viewer/controller/AdminController.java 45 290
show docs/security/2026-08-05-administrative-authorization.md 1 180
show docs/deployment/2026-07-02-buyer-deployment-integration-playbook.md 1 330
show AGENTS.md 75 155
show CHANGELOG.md 1 150

echo '== All cleanup-queue, cleanup retry, scheduler, and durable-cleanup assertions =='
rg -n -i -C3 \
  'ArtifactCleanupQueue|process-local.*(queue|retry)|cleanup.*(queue|retry|cadence|metric|evidence)|pending.*cleanup|`@EnableScheduling`|durable cleanup' \
  AGENTS.md CHANGELOG.md docs src/main/java src/test/java \
  -g '*.md' -g '*.java' -g '*.yml' -g '*.yaml' || true

echo '== Parent-relative diffs for source, contracts, tests, and documentation =='
git diff --unified=18 "$parent" "$head" -- \
  src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java \
  src/main/java/com/clearfolio/viewer/repository/ConversionJobRepository.java \
  src/main/java/com/clearfolio/viewer/repository/ConversionJobStateStore.java \
  src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java \
  src/main/java/com/clearfolio/viewer/service/DefaultConversionWorker.java \
  src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryConcurrencyTest.java \
  src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryTenantScopeTest.java \
  src/test/java/com/clearfolio/viewer/service/TenantScopedRetryContractTest.java \
  AGENTS.md CHANGELOG.md \
  docs/deployment/2026-07-02-buyer-deployment-integration-playbook.md \
  docs/security/2026-08-05-administrative-authorization.md

echo '== Administrative mappings, permissions, and service calls =='
rg -n -C4 \
  '@(GetMapping|DeleteMapping|PostMapping)|TenantPermissions\.ADMIN_(READ|WRITE)|getJobsForTenant|deleteJob\(|retryDeadLettered\(' \
  src/main/java/com/clearfolio/viewer/controller/AdminController.java \
  src/main/java/com/clearfolio/viewer/service/DocumentConversionService.java \
  src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java

Length of output: 3264


🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='73d1f77c67e417c51aa7548b7a43d4aea6ad4463'
parent='26563218ae42eaa876c784fcf56b27f8cb810080'

show() {
  local file="$1" start="$2" end="$3"
  echo "===== ${file} lines ${start}-${end} ====="
  awk -v start="$start" -v end="$end" \
    'NR >= start && NR <= end { printf "%6d  %s\n", NR, $0 }' "$file"
}

show src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java 1 390
show src/main/java/com/clearfolio/viewer/repository/ConversionJobRepository.java 1 230
show src/main/java/com/clearfolio/viewer/repository/ConversionJobStateStore.java 1 180
show src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java 220 315
show src/main/java/com/clearfolio/viewer/service/DefaultConversionWorker.java 160 230
show src/main/java/com/clearfolio/viewer/controller/AdminController.java 45 290
show docs/security/2026-08-05-administrative-authorization.md 1 180
show docs/deployment/2026-07-02-buyer-deployment-integration-playbook.md 1 330
show AGENTS.md 75 155
show CHANGELOG.md 1 150

echo '== All cleanup-queue, cleanup retry, scheduler, and durable-cleanup assertions =='
rg -n -i -C3 \
  'ArtifactCleanupQueue|process-local.*(queue|retry)|cleanup.*(queue|retry|cadence|metric|evidence)|pending.*cleanup|`@EnableScheduling`|durable cleanup' \
  AGENTS.md CHANGELOG.md docs src/main/java src/test/java \
  -g '*.md' -g '*.java' -g '*.yml' -g '*.yaml' || true

echo '== Parent-relative diffs for source, contracts, tests, and documentation =='
git diff --unified=18 "$parent" "$head" -- \
  src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java \
  src/main/java/com/clearfolio/viewer/repository/ConversionJobRepository.java \
  src/main/java/com/clearfolio/viewer/repository/ConversionJobStateStore.java \
  src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java \
  src/main/java/com/clearfolio/viewer/service/DefaultConversionWorker.java \
  src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryConcurrencyTest.java \
  src/test/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepositoryTenantScopeTest.java \
  src/test/java/com/clearfolio/viewer/service/TenantScopedRetryContractTest.java \
  AGENTS.md CHANGELOG.md \
  docs/deployment/2026-07-02-buyer-deployment-integration-playbook.md \
  docs/security/2026-08-05-administrative-authorization.md

echo '== Administrative mappings, permissions, and service calls =='
rg -n -C4 \
  '@(GetMapping|DeleteMapping|PostMapping)|TenantPermissions\.ADMIN_(READ|WRITE)|getJobsForTenant|deleteJob\(|retryDeadLettered\(' \
  src/main/java/com/clearfolio/viewer/controller/AdminController.java \
  src/main/java/com/clearfolio/viewer/service/DocumentConversionService.java \
  src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java

Length of output: 234298


@seonghobae I reviewed exact head 73d1f77c67e417c51aa7548b7a43d4aea6ad4463 against parent 26563218ae42eaa876c784fcf56b27f8cb810080.

The requested parent is the merge base. The parent-relative scope contains 32 files.

I found one actionable issue.

[P1] Remove claims for the removed cleanup subsystem
docs/deployment/2026-07-02-buyer-deployment-integration-playbook.md Line 27, Line 76, and Line 170 state that ArtifactCleanupQueue retains failed deletions and performs bounded asynchronous retry.
docs/security/2026-08-05-administrative-authorization.md Line 34, Line 60, and Line 73 through Line 75 make the same implementation and test claims.

The reviewed source has no ArtifactCleanupQueue, cleanup retry worker, or scheduling enablement. DefaultDocumentConversionService.deleteArtifact only logs and suppresses an artifact-store deletion exception. These documents therefore overstate the current cleanup behavior and the available privacy-safe aggregate evidence.

State the current behavior accurately:

  • Tenant-owned repository deletion occurs before best-effort artifact removal.
  • A failed artifact deletion can leave orphaned bytes.
  • Issue #263 owns durable deletion receipts, an outbox, and a cleanup worker.
  • Production cutover remains blocked on durable cleanup evidence.

I verified these static boundaries:

  • AdminController requires signed claims before service access.
  • Listing uses admin:read. Delete and retry use admin:write.
  • Tenant-aware repository, state-store, and service defaults fail closed. They do not call global lookup or legacy mutation methods.
  • Tenant-scoped delete and retry conceal missing and cross-tenant objects.
  • Administrative audit output uses separate actor, tenant, and job HMAC domains. The captured-log tests reject raw UUID values and jobId=.
  • The config-tree import uses CLEARFOLIO_SECRET_CONFIG_DIR.
  • InMemoryConversionJobRepository permanently reserves UUIDs. It accepts an idempotent save only for the exact live object. It rejects distinct live and tombstoned UUID reuse before secondary-index work.
  • The deterministic retry-versus-save test rejects Tenant B’s replacement. It verifies that the enqueue path resolves Tenant A’s original object.
  • I found no process-local cleanup queue, global scheduling enablement, temporary workflow, or dependency change in the reviewed scope.

I could not run mvn -B --no-transfer-progress verify or python -m pytest -q scripts. This sandbox cannot execute repository compilers or interpreters. Test totals, zero failures/errors/skips, zero missed production lines/branches, and warning-free Javadocs are unverified. I did not treat absent CI, Security Scan, SAST, or fuzz evidence as passing. I did not approve or merge the draft.

You are interacting with an AI system.

@seonghobae
seonghobae changed the base branch from fix/pii-logging-16240128950440010639 to snapshot/buyer-parent-2ff43fc August 6, 2026 13:01

Copy link
Copy Markdown
Collaborator Author

@coderabbitai @opencode-agent @cwl-noema-review Please perform a fresh formal review of exact current head 19fbfc4b938530ff70fe152ab294813f1b3a1e8f against immutable base 2ff43fc1b280f6de2ca903695e5cbeef5c0b3c17. The effective diff is now the bounded 37-file administrator-security slice; duplicated parent-owned CI trigger/test changes were removed. Verify signed-claim enforcement, admin:read/admin:write least privilege, tenant-scoped atomic lookup/mutation, permanent UUID reservation, tenant-bound secondary indexes, privacy-safe domain-separated audit pseudonyms, null/blank tenant fail-closed behavior, and exact absence of the parent CI files from the diff. Fuzz run 31104076410 is pending and must not be treated as passing. The existing incomplete-artifact-cleanup thread remains valid and must stay unresolved until #282/#283 integrate. Submit current-head actionable findings or a formal review; a separate counted write-authorized independent approval remains mandatory.

@seonghobae
seonghobae changed the base branch from snapshot/buyer-parent-2ff43fc to fix/pii-logging-16240128950440010639 August 6, 2026 13:07
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='19fbfc4b938530ff70fe152ab294813f1b3a1e8f'
parent='2ff43fc1b280f6de2ca903695e5cbeef5c0b3c17'

echo '== Exact ancestry =='
git show -s --format='HEAD %H%nParents %P%nSubject %s' "$head"
printf 'merge_base='; git merge-base "$head" "$parent"
printf 'ahead_behind='; git rev-list --left-right --count "$parent...$head"

echo '== Parent-relative scope =='
git diff --name-status "$parent" "$head"
printf 'changed_file_count='
git diff --name-only "$parent" "$head" | awk 'NF { count++ } END { print count + 0 }'

echo '== Parent-owned CI and prohibited changed paths =='
git diff --name-only "$parent" "$head" | rg -n -i \
  '(^|/)(\.github/workflows|.*ci.*|.*workflow.*|.*trigger.*|.*action.*|pom\.xml|build\.gradle|package\.json|gradle\.properties|ArtifactCleanupQueue|ClearfolioViewerApplication)($|/)' \
  || true

echo '== Changed Java structural map =='
git diff --name-only "$parent" "$head" | while IFS= read -r file; do
  case "$file" in
    *.java)
      echo "--- $file ---"
      ast-grep outline "$file" --items all --view compact 2>/dev/null || true
      ;;
  esac
done

echo '== Core implementation outlines =='
for file in \
  src/main/java/com/clearfolio/viewer/auth/TenantAccessService.java \
  src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java \
  src/main/java/com/clearfolio/viewer/controller/AdminController.java \
  src/main/java/com/clearfolio/viewer/repository/ConversionJobRepository.java \
  src/main/java/com/clearfolio/viewer/repository/ConversionJobStateStore.java \
  src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java \
  src/main/java/com/clearfolio/viewer/service/DocumentConversionService.java \
  src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java \
  src/main/java/com/clearfolio/viewer/service/DefaultConversionWorker.java \
  src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java \
  src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java
do
  echo "--- $file ---"
  ast-grep outline "$file" --items all --view compact 2>/dev/null || true
done

echo '== Authorization, tenant lookup, mutation, identity, dispatch, audit, and log scan =='
rg -n -i -C 5 \
  'requireSigned\s*\(|ADMIN_(READ|WRITE)|findByTenantAndContentHash\s*\(|findByContentHash\s*\(|findAllByTenantId|deleteByTenantAndId|retryDeadLetteredForTenant|findOrStoreByContentHash|storeNewCandidate|reservation|tombston|existing == job|jobIndexLock|enqueue\s*\(|claimForProcessing\s*\(|findById\s*\(|forAdministrative(Job|Actor|Tenant)|jobFingerprint|deleteArtifact\s*\(|log\.(trace|debug|info|warn|error)\s*\(|jobId=|CLEARFOLIO_SECRET_CONFIG_DIR' \
  src/main/java src/test/java src/main/resources \
  -g '*.java' -g '*.yml' -g '*.yaml'

echo '== Relevant tests and docs =='
fd -t f . src/test/java | rg \
  '(AdminController|TenantAccessService|InMemoryConversionJobRepository|TenantScoped|AdministrativeAuditLogger|ArtifactDeletionPrivacy|BuyerDemoSecret|ConversionDownloadAuthorization)' \
  | sort

rg -n -i -C 4 \
  'admin:read|admin:write|tombstone|generation|best effort|orphaned|outbox|cleanup|CLEARFOLIO_SECRET_CONFIG_DIR|config.tree|Javadoc|zero.*(failure|error|skip|missed)' \
  AGENTS.md CHANGELOG.md docs \
  -g '*.md' -g '*.yaml' || true

echo '== Parent-relative diff for security-sensitive files =='
git diff --unified=24 "$parent" "$head" -- \
  AGENTS.md CHANGELOG.md \
  src/main/java/com/clearfolio/viewer/auth/TenantAccessService.java \
  src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java \
  src/main/java/com/clearfolio/viewer/controller/AdminController.java \
  src/main/java/com/clearfolio/viewer/repository/ConversionJobRepository.java \
  src/main/java/com/clearfolio/viewer/repository/ConversionJobStateStore.java \
  src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java \
  src/main/java/com/clearfolio/viewer/service/DocumentConversionService.java \
  src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java \
  src/main/java/com/clearfolio/viewer/service/DefaultConversionWorker.java \
  src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java \
  src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java \
  src/main/resources/application.yml \
  src/main/resources/application-buyer-demo.yml \
  ':(glob)src/test/java/**/*.java' \
  ':(glob)docs/security/*administrative*' \
  ':(glob)docs/deployment/*buyer*'

Length of output: 107352


🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='19fbfc4b938530ff70fe152ab294813f1b3a1e8f'
parent='2ff43fc1b280f6de2ca903695e5cbeef5c0b3c17'

show_range() {
  local file="$1" start="$2" end="$3"
  printf '\n===== %s, lines %s-%s =====\n' "$file" "$start" "$end"
  awk -v start="$start" -v end="$end" \
    'NR >= start && NR <= end { printf "%6d  %s\n", NR, $0 }' "$file"
}

echo '== Exact ancestry and all changed paths =='
git show -s --format='HEAD %H%nParents %P%nSubject %s' "$head"
printf 'merge_base='; git merge-base "$head" "$parent"
printf 'ahead_behind='; git rev-list --left-right --count "$parent...$head"
git diff --name-status "$parent" "$head"
printf 'changed_file_count='
git diff --name-only "$parent" "$head" | awk 'NF { n++ } END { print n+0 }'

echo '== CI, workflow, build, cleanup, and scheduler paths in parent-relative diff =='
git diff --name-only "$parent" "$head" | rg -n -i \
  '(^|/)(\.github/|.*workflow.*|.*ci.*|.*trigger.*|.*action.*|pom\.xml|build\.gradle|package\.json|gradle\.properties|ArtifactCleanupQueue|ClearfolioViewerApplication)($|/)' \
  || true

echo '== Commit path changes after the prior reviewed head, if available =='
git log --format='%H %s' --ancestry-path \
  d57f1aa8153aa092607a147c9587c683f009ea06.."$head" || true
git diff --name-status d57f1aa8153aa092607a147c9587c683f009ea06 "$head" || true

echo '== Core authorization and repository implementation =='
show_range src/main/java/com/clearfolio/viewer/auth/TenantAccessService.java 1 240
show_range src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java 1 180
show_range src/main/java/com/clearfolio/viewer/controller/AdminController.java 1 340
show_range src/main/java/com/clearfolio/viewer/repository/ConversionJobRepository.java 1 260
show_range src/main/java/com/clearfolio/viewer/repository/ConversionJobStateStore.java 1 200
show_range src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java 1 460

echo '== Service mutation and worker dispatch =='
show_range src/main/java/com/clearfolio/viewer/service/DocumentConversionService.java 1 240
show_range src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java 210 370
show_range src/main/java/com/clearfolio/viewer/service/DefaultConversionWorker.java 150 245

echo '== Audit and configuration =='
show_range src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java 1 260
show_range src/main/java/com/clearfolio/viewer/security/AuditPseudonymizer.java 1 290
show_range src/main/resources/application.yml 1 220
show_range src/main/resources/application-buyer-demo.yml 1 180

echo '== Verify production callers of scoped and legacy content lookups =='
rg -n -C 5 \
  '\.(findByTenantAndContentHash|findByContentHash)\s*\(' \
  src/main/java -g '*.java' || true

echo '== Direct logging in the changed administrative paths =='
rg -n -C 3 \
  'log\.(trace|debug|info|warn|error)\s*\(|LOGGER\.(trace|debug|info|warn|error)\s*\(' \
  src/main/java/com/clearfolio/viewer/audit/AdministrativeAuditLogger.java \
  src/main/java/com/clearfolio/viewer/controller/AdminController.java \
  src/main/java/com/clearfolio/viewer/repository/InMemoryConversionJobRepository.java \
  src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java \
  -g '*.java' || true

Length of output: 102353


@seonghobae Formal static review completed for exact head 19fbfc4b938530ff70fe152ab294813f1b3a1e8f.

I found no new actionable blocker in the 37-file administrator-security slice.

The immutable base 2ff43fc1b280f6de2ca903695e5cbeef5c0b3c17 is the merge base. The head is 12 commits ahead and zero commits behind the base.

I verified these conditions:

  • The parent-relative diff contains 37 files.
  • The diff contains no parent-owned CI trigger or test files.
  • The diff contains no workflow, dependency-manifest, release, cleanup-queue, or application scheduling change.
  • TenantAccessService.requireSigned fails closed when the signing verifier is absent, weak, malformed, expired, or invalid.
  • AdminController performs signed-claim authorization before service access.
  • Listing requires admin:read.
  • Delete and retry require admin:write.
  • Tenant-scoped repository, state-store, and service defaults fail closed. They do not use global lookup or legacy mutation paths.
  • Missing, null, and cross-tenant targets remain concealed at the scoped boundaries.
  • findByTenantAndContentHash rejects null and blank tenant identifiers before it creates a tenant-content key.
  • Only the explicit legacy findByContentHash path selects buyer-demo.
  • The in-memory repository reserves accepted UUIDs permanently.
  • Only the exact stored live object is idempotent.
  • A distinct live or tombstoned UUID fails before candidate content-hash or tenant-content-index access.
  • Tenant-scoped deletion removes the live record and index but retains the UUID reservation.
  • The deterministic retry-versus-save regression rejects the replacement and preserves Tenant A’s original object for worker dispatch.
  • Administrative audit evidence uses separate HMAC domains for actor, tenant, and job fingerprints.
  • The artifact-deletion failure warning is controlled and contains no raw identifier or attached throwable.
  • Config-tree secret loading uses CLEARFOLIO_SECRET_CONFIG_DIR.

The incomplete artifact-cleanup finding remains valid and unresolved. This slice does not provide transactional receipts or an outbox, a retry worker, restart-safe cleanup, or aggregate cleanup evidence. Keep that finding open until #282 and #283 integrate.

I did not run Maven or Python commands in this sandbox. Exact test totals, skips, coverage, compiler warnings, and Javadoc warnings remain unverified.

Fuzz run 31104076410 is pending. I did not treat it as passing. CI, Security Scan, SAST, Strix, protected-main gates, and counted independent write-authorized approval remain required.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@opencode-agent @cwl-noema-review Please independently review exact current head 2e8e5ad46da7bb648e9e3f8a394c29d158345c5e relative to parent #270 exact head c1239e20b048a582b19cb9eeb90f2c29f7c8f814. Treat all predecessor-head checks, reviews, and approvals as stale.

Verify the complete tenant-administration and durable deletion boundary: signed tenant claims and admin:read/admin:write; fail-closed scoped repository/service defaults; atomic tenant-scoped list/delete/retry; permanent UUID reservation and tenant/generation-bound queued retry; domain-separated privacy-safe audit fingerprints; config-tree-only signing secrets; receipt-first metadata tombstones; append-only restart replay; bounded retry scheduling; exact digest/generation fencing; cross-tenant isolation; cleanup failure metrics; corruption and illegal-transition rejection; and documentation/CHANGELOG accuracy.

Exact-head evidence already completed successfully: CI run 31106968192 with exact-head Maven verify, synthetic merge compatibility, buyer-readiness scripts, 604 tests with zero failures/errors/skips, all JaCoCo line/branch gates met, and warning-free Javadocs; fuzz run 31106968335 also succeeded. Unresolved inline threads are zero. Security Scan, SAST Semgrep, Strix, formal exact-head reviews, and counted independent write-authorized approval remain absent and must not be inferred from status-only evidence. Keep Draft and do not merge or publish.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='2e8e5ad46da7bb648e9e3f8a394c29d158345c5e'
parent='c1239e20b048a582b19cb9eeb90f2c29f7c8f814'

echo '== Exact ancestry =='
git show -s --format='HEAD %H%nParents %P%nSubject %s' "$head"
printf 'merge_base='; git merge-base "$head" "$parent"
printf 'ahead_behind='; git rev-list --left-right --count "$parent...$head"

echo '== Parent-relative scope =='
git diff --name-status "$parent" "$head"
printf 'changed_file_count='
git diff --name-only "$parent" "$head" | awk 'NF { count++ } END { print count + 0 }'

echo '== Prohibited paths =='
git diff --name-only "$parent" "$head" | rg -n -i \
  '(^|/)(\.github/|.*workflow.*|.*ci.*|pom\.xml|build\.gradle|package\.json|gradle\.properties)($|/)' || true

echo '== Changed Java structural map =='
git diff --name-only "$parent" "$head" | while IFS= read -r file; do
  case "$file" in
    *.java)
      echo "--- $file ---"
      ast-grep outline "$file" --items all --view compact 2>/dev/null || true
      ;;
  esac
done

Length of output: 12981


🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='2e8e5ad46da7bb648e9e3f8a394c29d158345c5e'
parent='c1239e20b048a582b19cb9eeb90f2c29f7c8f814'

echo '== Workflow diff =='
git diff --unified=10 "$parent" "$head" -- .github/workflows/ci.yml

echo '== Deleted script content (parent side) =='
git show "$parent:scripts/test_ci_workflow_stack_coverage.py" | head -100

echo '== Was this script referenced anywhere? =='
git show "$parent" --stat | rg -i 'test_ci_workflow_stack_coverage' || true
rg -n 'test_ci_workflow_stack_coverage' -- .github 2>/dev/null || true
git show "$head:.github/workflows/ci.yml" | rg -n 'test_ci_workflow_stack_coverage|pytest' || true
git show "$parent:.github/workflows/ci.yml" | rg -n 'test_ci_workflow_stack_coverage|pytest' || true

echo '== Commit list on this branch relative to parent =='
git log --format='%H %s' --ancestry-path "${parent}..${head}"

Length of output: 2957


@seonghobae Independent review of exact head 2e8e5ad46da7bb648e9e3f8a394c29d158345c5e found two blocking issues. I stopped the deep line-level review of the new durable-deletion lifecycle code until these are resolved.

[P0] The stated parent is not the merge base. This branch is 32 commits behind the real parent head.

git merge-base 2e8e5ad46da7bb648e9e3f8a394c29d158345c5e c1239e20b048a582b19cb9eeb90f2c29f7c8f814 returns 2ff43fc1b280f6de2ca903695e5cbeef5c0b3c17, not c1239e20b048a582b19cb9eeb90f2c29f7c8f814.

git rev-list --left-right --count c1239e20b048a582b19cb9eeb90f2c29f7c8f814...2e8e5ad46da7bb648e9e3f8a394c29d158345c5e returns 2 32. Read this result in order: the head has 2 commits absent from the parent, and the parent has 32 commits absent from the head.

This branch does not contain the last 32 commits of parent #270. The parent-relative diff against the true merge base contains 74 changed files, not the stated 37-file slice. I cannot confirm scope boundaries, or trust that removed parent security/CI contracts stay intact, until this branch rebases onto the current parent exact head and the comparison becomes 0 commits behind.

[P1] The workflow change removes exact-head CI for this stacked PR, and the branch deletes the regression test that guarded against this exact change.

.github/workflows/ci.yml changes from:

pull_request: {}

to:

pull_request:
  branches: [main]

This PR targets a non-main base. After this change, CI does not run automatically on pushes to this PR. This removes the exact-head CI evidence this review thread has relied on for every prior head.

The deleted file scripts/test_ci_workflow_stack_coverage.py contained the two assertions that prevented this exact regression:

assert "  pull_request: {}" in workflow
assert "  pull_request:\n    branches: [main]" not in workflow

Restore the broad pull_request: {} trigger, or restore the deleted regression test if the trigger change is intentional and explain the rationale. Do not remove the test without preserving its guarantee some other way.

I did not review the new lifecycle files (ArtifactDeletionCoordinator, ArtifactDeletionLedger, ArtifactDeletionReceiptStore, DurableDocumentDeletionService, and related tests) in detail in this pass, because the parent mismatch makes any conclusion unreliable. Request a fresh review after the branch reconciles with the actual parent #270 exact head and reports 0 commits behind.

I did not run Maven or Python in this sandbox. I did not treat CI run 31106968192, fuzz run 31106968335, or any other status as passing evidence beyond what this repository inspection independently confirmed. I did not infer independent approval. Keep Draft.

(´・ω・`) ??? — something's off here, better to check together

You are interacting with an AI system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant