agent: add deterministic PVC binding diagnosis - #216
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Register diagnose_pvc with cluster_read/structured_yaml/_ALL_SURFACES. Bind-phase: Bound/Lost do a single GET. Pending adds events (once) and StorageClass LIST (once, skipped when storageClassName is explicitly ''). ApiStatusError on secondary reads becomes a gaps[] entry; transport and decoding failures remain tool errors. Follow mirrors to the persistentvolumeclaims describe pane. Add StorageClass to builtin API meta table. Docs updated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Finding 1 (Important) — Pending pre-bound PVCs: - Add _check_prebound rule (pvc.awaiting_prebound_volume, severity=warning, confidence=high) after static-binding check in core/pvc_analysis.py. Provisioning-failure events still take precedence. Class/default resolution skipped when spec.volumeName is set. - Executor _diagnose_pvc skips StorageClass LIST for pre-bound claims but still fetches events once. - Update docstring rule list (pre-bound at position 4, remove phantom rule 8). Finding 2 (Minor) — Default StorageClass annotation exact semantics: - Change _storage_class_is_default from value.lower() == 'true' to exact value == 'true'; only literal lowercase 'true' matches Kubernetes semantics. Finding 3 (Minor) — Failure event selection and evidence: - Sort failure events by last_seen desc, count desc, reason/message asc. - Surface event.count always; event.last_seen when non-empty. Finding 4 (Minor) — Executor event projection coverage: - PVCDiagnosisKube.list_events_for now records UID in calls tuple. - Add integration tests: UID scoping, Warning-only filter, 240-char clamp, prebound path, ApiStatusError -> gap, non-ApiStatusError -> ERROR:. Finding 5 (Minor) — Analyzer docstring updated. Gate: ruff clean, mypy clean, 4394 passed / 21 skipped, tach ✅ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…n PVC analyzer
Two high-confidence Minor findings addressed:
1. Canonical static pre-binding order (pvc_analysis.py)
- Move _check_prebound before _check_static_binding so a PVC with
volume_name='my-pv' and storage_class_name='' correctly emits
pvc.awaiting_prebound_volume (spec.volumeName evidence, named-PV
next checks) instead of pvc.awaiting_static_volume.
- Updated rule-priority docstring to reflect new canonical order (3 to 4).
- Regression tests: test_prebound_with_empty_storage_class_* (3 tests).
2. Event series/time semantics
a. Pure analyzer (core/pvc_analysis.py): replace lexicographic string
sort on last_seen with chronological datetime comparison via new
_event_instant_from_snapshot helper (stdlib datetime only; no
tools import; epoch fallback for empty/invalid timestamps).
Regression tests: RFC3339-offset and fractional-second cases.
b. Executor (tools/executor.py): replace inline ev.get count /
ev.get lastTimestamp with canonical _event_count/_event_last_seen
helpers imported from korvid.tools.diagnose so that
events.k8s.io/v1 series.count/series.lastObservedTime and all
fallback timestamp fields are projected correctly.
Integration test: test_series_count_and_last_observed_time_reach_finding.
Gate: ruff clean, mypy strict clean, tach clean,
4401 passed / 21 skipped (make check).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds deterministic, structured PVC binding diagnosis across agent and MCP surfaces.
Changes:
- Introduces shared finding/report contracts and PVC binding rules.
- Adds StorageClass projection and evidence collection.
- Registers, documents, and tests
diagnose_pvcand follow navigation.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/korvid/core/findings.py |
Defines shared analysis contracts. |
src/korvid/core/pvc_analysis.py |
Implements PVC binding rules. |
src/korvid/core/service_analysis.py |
Adopts shared contracts. |
src/korvid/k8s/models.py |
Adds StorageClass summaries. |
src/korvid/tools/executor.py |
Collects and analyzes PVC evidence. |
src/korvid/tools/registry.py |
Registers diagnose_pvc. |
src/korvid/tools/follow.py |
Adds PVC follow navigation. |
src/korvid/agent/prompts.py |
Adds PVC tool guidance. |
docs/agent.md |
Documents agent support. |
docs/mcp.md |
Documents MCP behavior. |
tests/core/test_pvc_analysis.py |
Covers PVC analysis rules. |
tests/core/test_service_analysis.py |
Verifies contract compatibility. |
tests/k8s/test_models.py |
Tests StorageClass projection. |
tests/tools/test_executor.py |
Tests PVC execution behavior. |
tests/tools/test_follow.py |
Tests PVC follow behavior. |
tests/tools/test_list_resources.py |
Tests StorageClass rendering. |
tests/tools/test_registry.py |
Tests registration and format. |
tests/agent/test_profiles.py |
Tests compact profile descriptions. |
Suppressed comments (2)
src/korvid/tools/executor.py:1671
- This also turns every StorageClass API status (including throttling and server failures) into an RBAC evidence gap. The documented contract only makes denied optional reads non-fatal; re-raise statuses other than 403 so operational failures remain tool errors.
except ApiStatusError as exc:
return (), EvidenceGap("storageclasses", _api_gap_reason(exc))
src/korvid/core/pvc_analysis.py:403
- This generic branch also reports the nonexistent
spec.volumeBindingModepath for a StorageClass. Use the resource's actual top-level field name so consumers can trace the evidence.
_evidence(
resolved_sc.identity,
"spec.volumeBindingMode",
resolved_sc.volume_binding_mode,
),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
리뷰 요약
pvc.binding 결정론적 분석기와 diagnose_pvc 툴 추가는 전반적으로 설계가 견고합니다. 규칙 우선순위가 docstring과 실제 호출 순서(_check_phase → _check_failure_events → _check_prebound → _check_static_binding → _resolve_storage_class)와 일치하고, storageclasses evidence gap이 클래스 해석 규칙을 정확히 차단해 incomplete로 떨어지는 처리도 올바릅니다. _event_instant_from_snapshot 기반 정렬도 Python의 stable sort 특성상 count/reason/message tiebreak이 보존되어 결정론이 유지됩니다. 테스트 커버리지(규칙별 단위 테스트, StorageClass 프로젝션, follow/registry)도 충실합니다.
아래 2건은 기존 봇 리뷰에서 지적되지 않은 사항입니다.
1. src/korvid/tools/executor.py:1680 — Severity: Warning
문제: StorageClass LIST 결과를 isinstance 필터로 조용히 버리기 때문에, 프로젝션이 실패하면 evidence gap이 아니라 사실과 다른 확정 finding(pvc.no_default_storage_class 또는 pvc.storage_class_not_found)이 생성됩니다.
for s in summaries
if isinstance(s, StorageClassSummary)
)
return classes, NoneLIST 자체는 성공했으므로 class_gap은 None이고, _resolve_default_class는 빈 목록을 "클러스터에 기본 StorageClass가 없다"는 high-confidence 근거로 해석합니다. summary_for 디스패치가 group/apiVersion 변화(예: 향후 storage.k8s.io/v1beta1 경로 추가, custom column 파이프라인 변경)로 GenericSummary를 돌려주는 순간, 사용자에게는 존재하지 않는 문제에 대한 Warning이 결정론적으로 출력됩니다.
수정: 필터로 인해 항목이 유실되면 조용히 넘어가지 말고 evidence gap으로 승격시키십시오.
classes = tuple(
StorageClassSnapshot(...)
for s in summaries
if isinstance(s, StorageClassSummary)
)
if len(classes) != len(summaries):
return classes, EvidenceGap(
"storageclasses",
f"{len(summaries) - len(classes)} StorageClass objects could not be projected",
)
return classes, None2. src/korvid/core/pvc_analysis.py:44 — Severity: Suggestion
문제: PVCBindingSnapshot의 requested_storage와 access_modes는 _pvc_binding_snapshot에서 파싱되어 채워지지만 분석기 어느 규칙에서도 읽히지 않는 dead field입니다.
requested_storage: str
access_modes: tuple[str, ...]특히 pvc.awaiting_prebound_volume와 pvc.awaiting_static_volume는 "PV가 claim 요구사항을 만족하는지 확인하라"는 next_check를 내면서도, 이미 확보한 용량/접근모드 값을 evidence에서 누락시켜 사용자가 PV와 대조할 근거를 잃습니다.
수정: 두 static/pre-bound finding의 evidence에 실제 관측값을 추가하십시오. 예:
evidence=(
_evidence(pvc_id, "spec.volumeName", pvc.volume_name),
_evidence(pvc_id, "spec.resources.requests.storage", pvc.requested_storage),
_evidence(pvc_id, "spec.accessModes", ",".join(pvc.access_modes)),
),값을 쓰지 않을 계획이라면 스냅샷에서 두 필드를 제거해 계약을 실제 사용 범위와 일치시키는 편이 낫습니다.
버그/보안/성능 관점의 Critical 이슈는 발견되지 않았습니다. 읽기 전용 계약(write/Secret/PV fan-out 없음)과 _reject_slash_name 입력 검증도 유지되고 있습니다.
COMMENT
- Item 1: _pvc_event_evidence and _pvc_storage_classes now only convert status==403 to EvidenceGap; 401/404/429/5xx are re-raised so the tool executor returns ERROR instead of silently degrading analysis. Added parameterized tests for non-403 on both events and StorageClass. - Item 2: Added has_provisioning_failure_event(events) public helper in pvc_analysis (reuses _PROVISIONING_FAILURE_REASONS constant). After fetching events in _diagnose_pvc, if a decisive failure event exists the StorageClass LIST is skipped and analyze_pvc_binding is called immediately. Added executor tests proving failure event → GET+events only and that a StorageClass transport failure cannot mask it. - Item 3: Replaced 'spec.volumeBindingMode' evidence field with top-level 'volumeBindingMode' in _resolve_binding_mode for both WaitForFirstConsumer and generic pending findings. Added path-assertion tests. - Item 4: Extended StorageClassSummary with default_annotation_key / default_annotation_value fields (stable key wins when both are 'true'). Extended StorageClassSnapshot with the same fields (default ''). Executor _pvc_storage_classes carries both fields into snapshots. _resolve_default_class now emits evidence using metadata.annotations.<actual-key> and the literal observed value 'true' instead of the synthetic 'metadata.annotations=is-default'. Added model tests (stable/beta key preservation, priority, non-default empty fields) and analyzer evidence tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/korvid/core/pvc_analysis.py:333
- Kubernetes does not stop when multiple defaults exist: it selects the most recently created default StorageClass. Returning here makes a valid omitted-class PVC look blocked by the duplicate defaults and prevents diagnosis of the effective class's binding mode (for example,
WaitForFirstConsumer). Preserve each class's creation timestamp, resolve the newest default, and, if the duplicate-default warning is retained, report it alongside rather than instead of the effective binding result.
if len(defaults) > 1:
return _findings_report(
pvc_id,
gaps,
_finding(
"pvc.multiple_default_storage_classes",
"warning",
"high",
pvc_id,
explanation=(
"Multiple StorageClasses are marked as default; only one should be default."
src/korvid/tools/registry.py:674
- This read-contract description is stale after the failure/pre-bound optimization: the implementation also skips the StorageClass LIST for pre-bound claims and decisive provisioning-failure events. Since this schema is sent directly to agents, describe all skip cases so callers do not infer cluster reads that never occur.
"Deterministically check why a PersistentVolumeClaim is not Bound. "
"One GET for Bound/Lost claims; fetches Warning events and StorageClass "
"evidence for Pending claims (skips StorageClass LIST when "
"storageClassName is explicitly empty). Returns versioned findings with "
"explicit evidence gaps when reads are denied. "
"Follow opens persistentvolumeclaims describe. "
"Prefer this over get_resource/get_events when a PVC is stuck."
docs/mcp.md:56
- This operational contract says every Pending claim lists StorageClasses except the explicit-empty case, but the implementation also skips that LIST for pre-bound claims and when a decisive failure event already determines the finding. Document those cases so MCP operators can accurately predict required permissions and API traffic.
- **PVC binding diagnosis** (`diagnose_pvc`) is deterministic structured
YAML. For `Bound`/`Lost` claims only one GET is made. For `Pending` claims
it additionally fetches Warning events (once) and lists StorageClasses
(once, skipped when `storageClassName` is explicitly empty). RBAC denials
on either secondary read become `gaps[]` entries so the model can reason
about incomplete evidence; transport and decoding failures remain tool
errors. Follow opens the `persistentvolumeclaims` describe screen.
src/korvid/agent/prompts.py:151
- The small-model prompt lists only the explicit-empty skip, but
_diagnose_pvcalso avoids StorageClass reads for pre-bound claims and decisive failure events. Keep this compressed description aligned with the actual read contract so the model does not reason from nonexistent StorageClass evidence.
"diagnose_pvc": (
"Deterministic PVC binding diagnosis: checks Bound/Lost PVCs directly, "
"and for Pending claims pulls Warning events plus StorageClass "
"evidence (skips an empty storageClassName). Prefer first for a "
"Pending or stuck PVC."
docs/agent.md:17
- This overview likewise implies every Pending claim reads StorageClasses, although pre-bound and decisive-failure cases intentionally do not. Qualify the secondary reads as conditional so the user-facing agent documentation matches the implemented read contract.
denial); `diagnose_pvc` checks why a PersistentVolumeClaim is not Bound (one
GET for Bound/Lost, plus Warning events and StorageClass evidence for Pending,
with RBAC denials surfaced as evidence gaps) — projected evidence instead of
raw YAML dumps, which is where small local models otherwise fail. It can also drive the TUI itself — navigate views, apply filters,
src/korvid/core/pvc_analysis.py:480
- The new confidence suppression has no explanatory reason, contrary to the repository rule in
AGENTS.md:63that every# type: ignore[code]include one. Better, type these parameters with the sharedSeverityandConfidencealiases so mypy can validate callers and both suppressions can be removed.
severity=severity, # type: ignore[arg-type] # literal checked by tests
confidence=confidence, # type: ignore[arg-type]
There was a problem hiding this comment.
리뷰 요약
78fbca0 대비 이번 커밋(2b10a7e)은 이전 봇 리뷰들의 지적 세 가지를 정확히 해결했습니다.
ApiStatusError를 무조건 evidence gap 으로 삼키던 두 경로가if exc.status != 403: raise로 좁혀져(src/korvid/tools/executor.py:1647-1649,:1674-1676), 스로틀링·서버 오류 같은 운영 실패가 "RBAC 로 못 읽었다" 로 둔갑하지 않게 되었습니다. 문서화된 계약("거부된 optional read 만 non-fatal")과 이제 일치합니다.- StorageClass evidence 경로가 존재하지 않는
spec.volumeBindingMode에서 실제 top-level 필드volumeBindingMode로 교정되었습니다(src/korvid/core/pvc_analysis.py:394,:418). - multiple-default evidence 가
metadata.annotations/ 리터럴"is-default"라는 합성값 대신 실제 annotation 키·값을 싣도록StorageClassSummary에default_annotation_key/value가 추가되었고(src/korvid/k8s/models.py:322-340), stable 키 우선·"true"리터럴만 인정하는 규칙이 테스트로 고정되었습니다.
또한 has_provisioning_failure_event 를 analyzer 의 _PROVISIONING_FAILURE_REASONS 를 재사용하도록 export 해 reason 목록 중복 없이 결정적 실패 이벤트일 때 StorageClass LIST 를 건너뛰게 한 최적화도 타당합니다 — 그 경우 finding 은 규칙 우선순위 2번에서 이미 확정되므로 LIST 결과는 어차피 쓰이지 않습니다. 추가된 테스트(test_failure_event_skips_storage_class_list, test_storage_class_transport_error_cannot_mask_failure_event, 비-403 재발생 파라미터 테스트)도 회귀를 정확히 고정합니다. 로컬에서 tests/core/test_pvc_analysis.py tests/k8s/test_models.py tests/tools/test_executor.py 326건 전부 통과를 확인했습니다.
남은 지적은 아래 인라인 1건입니다. 이전 리뷰(#4880174020)에서 제기했고 이번 커밋이 바로 그 블록을 수정했음에도 미해결로 남은 사항입니다.
검토 항목별 정리
- 버그:
isinstance필터로 인한 조용한 유실이 evidence gap 이 아니라 사실과 다른 high-confidence finding 을 만듭니다(finding 1). 별건으로,_resolve_default_class가 default 가 2개 이상일 때 조기 return 하는 부분은 Kubernetes 가 실제로는 가장 최근에 생성된 default 를 선택한다는 점에서 여전히 논의 대상입니다 — Copilot 리뷰가 같은 지적을 했으므로 중복 인라인은 달지 않습니다. - 보안: 읽기 전용 계약(write/Secret/PV fan-out 없음)과
_reject_slash_name입력 검증이 유지됩니다. 비-403 재발생은 오히려 오류를 은폐하지 않는 방향이라 개선입니다. 회귀 없음. - 성능: 결정적 실패 이벤트·pre-bound·명시적 빈 클래스에서 cluster-wide StorageClass LIST 를 생략하므로 API 호출이 줄어듭니다. 회귀 없음.
- 에러 처리: 403 만 gap, 그 외 status 와 transport/decoding 실패는 tool error — 계약이 명확해졌습니다.
- 테스트: 신규 규칙·프로젝션·스킵 경로가 모두 단위 테스트로 덮여 있습니다. finding 1 을 반영하면 프로젝션 유실 시 gap 이 생기는지 확인하는 테스트도 함께 추가해 주세요.
Finding 1 (multiple defaults): add StorageClassSnapshot.created, pass s.created in executor, pick effective class by newest RFC3339 timestamp (name tie-break for equal timestamps). Emit both pvc.multiple_default_storage_classes and the binding-mode finding (pvc.waiting_for_first_consumer or pvc.provisioning_pending) together. Extract _resolve_binding_mode_finding helper for single/multi-default path sharing. Add five targeted TDD tests. Finding 2 (read-contract descriptions): update registry schema, SMALL_TOOL_DESCRIPTIONS (<=250), docs/agent.md, docs/mcp.md to accurately state Warning events are read for unresolved claims and StorageClasses are listed only when no failure event, pre-bound volume, or explicit-empty/static-binding evidence already determines the result. Finding 3 (type safety): import Severity/Confidence aliases from korvid.core.findings, type _finding parameters accordingly, remove both # type: ignore[arg-type] suppressions. Gate: ruff/mypy/tach clean; pytest 4436 passed, 21 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/korvid/core/pvc_analysis.py:428
- When the Events LIST is denied,
warning_eventsis empty but theeventsgap is passed into this branch, so the report still claims that provisioning "has not reported a specific failure." That conclusion is unsupported because failure events could not be read. Use the existinggapsargument to emit gap-aware wording (or return an incomplete report) instead.
f"PVC is not yet Bound with StorageClass '{resolved_sc.identity.name}' "
"(volumeBindingMode=Immediate); provisioning has not reported a "
"specific failure."
There was a problem hiding this comment.
리뷰 요약 (2b10a7e → 29fb2e4)
이번 두 커밋(a4a86e7, 29fb2e4)은 이전 라운드의 supressed 지적을 정확히 반영했습니다.
- multiple default StorageClass 처리 교정: Kubernetes 는 default 가 여러 개일 때 멈추지 않고 가장 최근 생성된 default 를 선택합니다.
_resolve_default_class가 조기 return 하던 것을 고쳐,pvc.multiple_default_storage_classes경고와 effective class 기준 binding-mode finding 을 함께 반환하도록 바뀌었습니다(src/korvid/core/pvc_analysis.py:328-356).WaitForFirstConsumer가 실제 유효 클래스일 때 사용자가 원인을 놓치던 문제가 해소됩니다. - 결정론 유지:
min(defaults, key=lambda sc: (-_sc_creation_instant(sc).timestamp(), sc.identity.name))은 timestamp 동률에서 이름 오름차순으로 확정되며, 입력 순서를 바꿔도 동일 결과임이test_multiple_defaults_name_tiebreak_is_deterministic로 고정되었습니다.created미존재/파싱 실패는_EPOCH로 떨어져 가장 오래된 것으로 취급되므로 예외 경로도 결정적입니다. created전달 경로 완결:StorageClassSummary.created(GenericSummary 상속) →StorageClassSnapshot.created연결이src/korvid/tools/executor.py:1685에서 이루어져 스냅샷이 실제metadata.creationTimestamp를 싣습니다.- 타입 정리:
Severity/Confidence를 export 해_finding의# type: ignore[arg-type]두 건을 제거한 것은 실제 타입 안전성 개선입니다. - 문서/스키마 stale 수정:
registry.py:668-675,docs/mcp.md,docs/agent.md,prompts.py가 실제 skip 조건(결정적 실패 이벤트 / pre-bound volume / explicit-empty·static binding)을 모두 서술하도록 갱신되어 에이전트가 하지 않는 클러스터 read 를 추론하지 않게 되었습니다.
로컬 검증: uv run pytest tests/core/test_pvc_analysis.py tests/tools/test_executor.py tests/k8s/test_models.py tests/tools/test_registry.py → 388 passed, uv run ruff check . → All checks passed. (전체 스위트는 httpx 등 미설치 의존성으로 12개 collection error 가 나지만 본 PR 변경과 무관합니다.)
검토 항목별 정리
- 버그: 아래 finding 1 (
isinstance필터의 조용한 유실) 이 이번 커밋에서 바로 그 블록을 수정했음에도 여전히 미해결입니다. - 보안: 읽기 전용 계약,
_reject_slash_name입력 검증, 403 만 gap 처리하는 규칙 모두 유지 — 회귀 없음. - 성능: 변경은 이미 가져온 in-memory 목록에 대한
min()한 번뿐이라 추가 API 호출이 없습니다 — 회귀 없음. - 에러 처리: finding 2 (미사용
gaps파라미터) 로 인해 events gap 상황에서의 wording 개선 여지가 남아 있습니다. - 테스트: 신규 5개 테스트가 dual-finding, 최신 default 선택, 이름 tiebreak, evidence/related 보존을 모두 덮고 있어 충실합니다.
2. src/korvid/core/pvc_analysis.py:392 — Severity: Suggestion
문제: 이번 커밋에서 새로 추출한 _resolve_binding_mode_finding 이 gaps 파라미터를 받지만 함수 본문에서 한 번도 읽지 않아, events LIST 가 거부된 상황에서도 "provisioning has not reported a specific failure" 라는 근거 없는 단정을 그대로 출력합니다.
def _resolve_binding_mode_finding(
pvc: PVCBindingSnapshot,
pvc_id: ResourceIdentity,
gaps: tuple[EvidenceGap, ...],
resolved_sc: StorageClassSnapshot,
) -> Finding:events gap 이 존재하면 Warning event 가 비어 있는 이유가 "실패가 없어서" 인지 "읽지 못해서" 인지 구분되지 않는데, 현재 explanation 은 전자를 확정합니다. 파라미터를 실제로 소비하거나 제거해 계약을 명확히 하십시오.
수정:
events_gap = any(g.source == "events" for g in gaps)
explanation = (
f"PVC is not yet Bound with StorageClass '{resolved_sc.identity.name}' "
"(volumeBindingMode=Immediate); "
+ (
"Warning events could not be read, so a provisioning failure cannot be ruled out."
if events_gap
else "provisioning has not reported a specific failure."
)
)(gap 을 반영하지 않기로 결정한다면 파라미터 자체를 시그니처에서 제거하는 편이 오해를 줄입니다.)
COMMENT
Issue 1 — Pending PVC missing UID: - _diagnose_pvc now raises ValueError (→ ERROR:) before calling list_events_for or list_objects when pvc.identity.uid is empty. - Bound/Lost phases skip that guard (no event call needed). - Executor regression tests: pending-missing-uid → ERROR, only GET occurred; bound/lost-missing-uid → healthy/findings (no events call). Issue 2 — StorageClass projection loss: - _pvc_storage_classes detects non-StorageClassSummary rows; if any exist, returns typed rows + EvidenceGap(source='storageclasses') with a count-only reason (no object names or content quoted). - Analyzer already promotes storageclasses gap to incomplete for no-default and named-class-not-found paths; new gap triggers that guard correctly. - Executor tests: mixed/all-untyped rows → storageclasses gap + incomplete; gap reason contains count, not object name. Issue 3 — Events gap + generic Immediate pending: - _resolve_binding_mode: Immediate mode + events gap → _incomplete_report instead of pvc.provisioning_pending (which falsely asserts no failure was reported). - WaitForFirstConsumer is deterministic; events gap never suppresses it. - _resolve_default_class multiple-defaults path: Immediate + events gap → only pvc.multiple_default_storage_classes emitted (not provisioning_pending). - Pure analyzer tests: events_gap+Immediate → incomplete; events_gap+WFFC → finding with gap; class-independent findings (no_default, missing_class, multiple_defaults, WFFC) coexist with events gap. - Executor regression: events 403 + Immediate → outcome=incomplete. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
리뷰 요약 (29fb2e4 → 73b890c)
이번 커밋(73b890c)은 지난 라운드의 두 지적을 정면으로 반영했습니다.
- 프로젝션 유실이 evidence gap 으로 승격:
_pvc_storage_classes가isinstance필터로 버려진 행 수를 세어EvidenceGap("storageclasses", "N of M StorageClass row(s) could not be projected")를 반환합니다(src/korvid/tools/executor.py:1677-1694). 이제_resolve_storage_class의has_sc_gap경로가 작동해pvc.no_default_storage_class/pvc.storage_class_not_found라는 사실과 다른 high-confidence finding 대신incomplete가 나옵니다 — 정확히 요청한 수정입니다. - 미사용
gaps파라미터 해소:_resolve_binding_mode가 Immediate 모드 + events gap 조합에서_incomplete_report를 반환하도록 바뀌어(src/korvid/core/pvc_analysis.py:395-401), "provisioning has not reported a specific failure" 라는 근거 없는 단정이 직렬화되지 않습니다. WaitForFirstConsumer 는 이벤트 근거가 필요 없는 결정론적 판정이라 gap 과 무관하게 유지한 구분도 타당합니다.
로컬 검증: uv run pytest tests/core/test_pvc_analysis.py tests/tools/test_executor.py -q → 239 passed. 신규 테스트가 프로젝션 유실 gap, events gap 하 Immediate/WFC 분기, uid 부재 오류를 모두 덮습니다.
검토 항목별 정리
- 버그: 아래 finding 1 (multiple-default 경로의 outcome 불일치).
- 보안: 읽기 전용 계약과
_reject_slash_name검증 유지. uid 스코핑 강제는 다른 claim 의 이벤트 혼입을 막는 방향이라 개선입니다 — 다만 실패 처리 방식은 finding 2 참고. - 성능: 추가 API 호출 없음. 유실 분기에서 tuple 생성이 중복되지만 O(N) 한 번으로 동일합니다(중복 코드는 아래 finding 3).
- 에러 처리: finding 2.
- 테스트: 신규 경로 커버리지 충분합니다.
3. src/korvid/tools/executor.py:1677 — Severity: Suggestion
문제: 유실이 있는 분기와 없는 분기가 완전히 동일한 StorageClassSnapshot(...) 생성 블록을 두 번 복제해, 향후 필드가 추가되면 한쪽만 갱신될 위험이 있습니다.
if untyped_count:
...
classes = tuple(
StorageClassSnapshot(
identity=ResourceIdentity("StorageClass", "", s.name, s.uid or ""),
...수정: 스냅샷 생성을 한 번만 하고 gap 만 조건부로 만드십시오.
classes = tuple(_sc_snapshot(s) for s in typed)
if untyped_count:
return classes, EvidenceGap(
"storageclasses",
f"{untyped_count} of {len(summaries)} StorageClass row(s) could not be projected",
)
return classes, NoneCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/korvid/core/pvc_analysis.py:348
StorageClassSnapshotallowsis_default=Truewhile both annotation fields retain their empty defaults (the test helper attests/core/test_pvc_analysis.py:38-50does this), so this emits fabricated evidence with fieldmetadata.annotations.and value"". That violates the analyzer’s exact-evidence contract. Enforce the snapshot invariant by requiring the observed default annotation key/value wheneveris_defaultis true (and update fixtures), or treat a snapshot lacking them as incomplete instead of serializing a nonexistent field.
f"metadata.annotations.{sc.default_annotation_key}",
sc.default_annotation_value,
There was a problem hiding this comment.
리뷰 요약 (73b890c → 2a27314)
이번 커밋의 두 변경은 모두 방향이 옳습니다.
- multiple-default + Immediate + events gap 경로의 outcome 불일치 해소: 이전 라운드에서 지적한, 단일 default 경로는
incomplete인데 duplicate-default 경로는_findings_report(=complete)를 돌려주던 비대칭이_incomplete_findings_report도입으로 정리되었습니다(src/korvid/core/pvc_analysis.py:355-357).pvc.multiple_default_storage_classes경고는 이벤트 근거가 필요 없는 결정론적 판정이므로 유지하면서 outcome 만incomplete로 낮춘 선택이 정확합니다.test_events_gap_with_immediate_class_is_incomplete_for_single_and_multiple_defaults가 두 경로를 한 테스트에서 함께 고정한 것도 재발 방지에 적절합니다. - events 조회의 uid 스코핑 강제:
uid=pvc.identity.uid or None이 사라지고 호출부에서 uid 부재를 이미ValueError로 차단한 뒤 non-empty uid 를 명시 인자로 전달합니다(src/korvid/tools/executor.py:1725). 동명(同名) 재생성 claim 의 이벤트가 섞일 여지가 없어져 근거 정확성이 올라갑니다.
검토 항목별 정리
- 버그: 이번 커밋 범위에서 신규 버그는 발견하지 못했습니다. duplicate-default 시
min(...)기반 effective 선택,_EPOCHfallback, 이름 tiebreak 모두 그대로 결정론적입니다. - 보안: 읽기 전용 계약과
_reject_slash_name검증 유지. uid 강제는 오히려 evidence 오염 위험을 줄이는 방향입니다 — 회귀 없음. - 성능: 추가 API 호출 없음.
any(...)한 번이 늘 뿐입니다 — 회귀 없음. - 에러 처리: 403 만 gap, 그 외 status 는 재발생, uid 부재는 tool error — 계약이 일관됩니다.
- 테스트: 신규 테스트가 단일/복수 default 두 분기를 모두 덮고, 기존 테스트에도
report.outcome == "incomplete"assertion 이 보강되었습니다. 충분합니다.
남은 지적은 인라인 3건이며 모두 Suggestion(유지보수성) 수준입니다. 이전 라운드에서 제기했던 _pvc_storage_classes 의 스냅샷 생성 블록 중복(executor.py:1683-1707)은 이번 커밋이 해당 블록을 건드리지 않아 재게시하지 않습니다.
COMMENT
Keep StorageClassSnapshot defaults canonical by requiring a real default annotation key and literal true value. Update test helpers to build valid default snapshots and add invariant tests so fabricated metadata.annotations. paths cannot be constructed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/korvid/core/pvc_analysis.py:424
- This decision ignores
resolved_sc.provisioner. Forprovisioner: kubernetes.io/no-provisioner, Kubernetes performs no automatic provisioning: the Immediate path incorrectly reports provisioning pending and suggests provisioner logs, while the WaitForFirstConsumer path says a Pod will make the claim bind without mentioning that a matching static PV must already exist. Add a static/no-provisioner rule before branching on binding mode (and retain the consumer-scheduling detail for WFFC).
if resolved_sc.volume_binding_mode == "WaitForFirstConsumer":
return _finding(
There was a problem hiding this comment.
리뷰 요약 (2a27314 → 2e429b7)
이번 커밋은 직전 라운드에서 지적된 마지막 정확성 이슈, 즉 StorageClassSnapshot(is_default=True) 이면서 annotation 필드가 비어 있을 때 metadata.annotations. 와 빈 값이라는 존재하지 않는 evidence 가 직렬화되던 문제를 계약 수준에서 차단했습니다.
src/korvid/core/pvc_analysis.py:70-73의__post_init__가is_default=True인데default_annotation_key가 비었거나default_annotation_value != "true"인 스냅샷을ValueError로 거부합니다. 이로써pvc_analysis.py:355-359의 multiple-default evidence 생성은 항상 실제 관측된 annotation key/value 만 싣게 되어 analyzer 의 exact-evidence 계약이 타입 수준에서 보장됩니다.- 프로덕션 경로도 정합합니다:
src/korvid/k8s/models.py:330-334의_storage_class_default_annotation은 리터럴"true"일 때만(True, key, value)를 돌려주므로,executor.py:1683-1707의 Summary→Snapshot 변환에서 새 invariant 를 위반할 수 없습니다. 즉 이번 변경은 기존 동작을 깨지 않으면서 잘못된 상태만 막습니다. - 테스트 헬퍼
_class가is_default=True일 때 기본 annotation 을 자동 채우도록 갱신되어 기존 픽스처가 계약과 일치하게 되었고, 신규 파라미터 테스트test_storage_class_default_annotation_invariant_rejects_invalid_snapshot이 (빈 key / 빈 value /"false") 세 가지 위반 케이스를 모두 고정합니다.
로컬 검증: uv run pytest tests/core/test_pvc_analysis.py tests/tools/test_executor.py tests/k8s/test_models.py tests/tools/test_list_resources.py -q → 365 passed.
검토 항목별 정리
- 버그: 이번 커밋 범위에서 신규 버그 없음.
frozen=True, slots=True데이터클래스에서도__post_init__는 정상 동작하며 (object.__setattr__를 쓰지 않고 검증만 하므로) 문제 없습니다. - 보안: 읽기 전용 계약,
_reject_slash_name검증, 403 만 evidence gap 처리 규칙 모두 그대로 유지 — 회귀 없음. - 성능: 스냅샷 생성 시 boolean 검사 한 번뿐, 추가 API 호출 없음 — 회귀 없음.
- 에러 처리: 아래 finding 1 참고 (Suggestion 수준).
- 테스트: 위반 케이스 3종 파라미터화로 충분히 덮였습니다.
1. src/korvid/k8s/models.py:344 — Severity: Suggestion
문제: 새 invariant 가 StorageClassSnapshot 에만 걸려 있고 그 원천인 StorageClassSummary 에는 없어서, from_manifest 이외의 경로로 만들어진 Summary 가 들어오면 검증이 _pvc_storage_classes 변환 지점에서야 터지고 evidence gap 이 아니라 불투명한 tool error 로 표면화됩니다.
class StorageClassSummary(GenericSummary):
is_default: bool = False
default_annotation_key: str = ""
default_annotation_value: str = ""실제로 tests/tools/test_list_resources.py:181-190 은 is_default=True 이면서 annotation 필드를 비운 Summary 를 그대로 만들 수 있습니다(현재는 summary_facts 렌더링만 하므로 통과). 동일한 형태가 executor.py:1683 변환에 도달하면 ValueError 가 _diagnose_pvc 전체를 실패시킵니다.
수정: 동일 invariant 를 원천에도 걸어 위반을 생성 지점에서 잡으십시오.
def __post_init__(self) -> None:
if self.is_default and (
not self.default_annotation_key or self.default_annotation_value != "true"
):
raise ValueError(
"StorageClassSummary(is_default=True) requires the observed default "
"annotation key and value='true'."
)(tests/tools/test_list_resources.py:181 과 tests/tools/test_executor.py:3391 픽스처도 함께 갱신 필요.)
이전 라운드에서 제기했던 _pvc_storage_classes 의 스냅샷 생성 블록 중복(src/korvid/tools/executor.py:1683-1707)은 이번 커밋이 해당 블록을 건드리지 않아 재게시하지 않습니다. 남은 항목이 모두 Suggestion 수준이고 목표한 정확성 이슈가 해결되었으므로 승인합니다.
1. kubernetes.io/no-provisioner correctness: add _no_provisioner_finding() emitting pvc.awaiting_static_volume before generic binding-mode branching. Evidence cites StorageClass provisioner and volumeBindingMode fields. Immediate: does not suggest provisioner logs. WaitForFirstConsumer: retains scheduling/topology detail and states a static PV must exist. Finding is independent of events gap. Add 7 new pure tests. 2. Consolidate conclusiveness logic: extract _binding_mode_is_conclusive(sc, gaps) used by _resolve_binding_mode and _resolve_default_class. No-provisioner and WaitForFirstConsumer are always conclusive; Immediate requires readable events. 3. Consolidate incomplete report helpers: _incomplete_report now delegates to _incomplete_findings_report, eliminating the duplicated AnalysisReport construction. 4. Remove duplicate UID argument: _pvc_event_evidence(self, pvc) reads pvc.identity.uid directly. Caller ValueError guard remains unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/korvid/core/pvc_analysis.py:158
- This critical finding says
spec.volumeNameis empty, but its evidence records onlystatus.phase. That leaves the field that makes the state inconsistent out of the structured report, contrary to the exact-evidence contract. Include the observed empty field alongside the phase.
evidence=(_evidence(pvc_id, "status.phase", "Bound"),),
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
리뷰 요약 (2e429b7 → cea9436)
이번 두 커밋(116f760, cea9436)은 직전 라운드에서 Copilot 이 suppressed 로 남긴 두 지적을 정확히 반영했습니다.
kubernetes.io/no-provisioner오진 교정:_resolve_binding_mode가 binding mode 분기 이전에 provisioner 를 먼저 검사하도록 바뀌어(src/korvid/core/pvc_analysis.py:409-410), Immediate 모드에서 "provisioning 이 아직 실패를 보고하지 않았다 / provisioner pod 로그를 보라" 는 사실과 다른 안내가 사라졌습니다. WFFC 경로도 "Pod 가 스케줄되면 바인딩된다" 는 설명에 "매칭되는 static PV 가 이미 존재해야 한다" 를 함께 실어 Copilot 이 요구한 detail 보존 조건을 충족합니다.pvc.bound_without_volumeevidence 보강: 불일치를 만드는 필드 자체인spec.volumeName의 관측값(빈 문자열)이 evidence 에 추가되어(pvc_analysis.py:158-161) exact-evidence 계약이 지켜집니다.- 결정론/게이팅 일원화:
_binding_mode_is_conclusive로 single-default·multiple-default 두 경로가 동일한 판정을 쓰게 되었고, no-provisioner 는 events gap 과 무관하게 conclusive 로 처리됩니다. 이는 타당합니다 — 존재하지 않는 provisioner 의 실패 이벤트를 기다릴 이유가 없기 때문입니다. - uid 인자 중복 제거:
_pvc_event_evidence(pvc, uid)→_pvc_event_evidence(pvc)로 정리되어 호출부에서 검증한pvc.identity.uid와 다른 값이 전달될 여지가 사라졌습니다.
로컬 검증: uv run pytest tests/core/test_pvc_analysis.py tests/tools/test_executor.py -q → 251 passed.
검토 항목별 정리
- 버그: Critical 은 없습니다. 다만 아래 finding 1 의 rule_id 충돌은 구조화 출력 소비자 관점의 실질적 문제입니다.
- 보안: 읽기 전용 계약,
_reject_slash_name검증, 403 만 evidence gap 처리 규칙 모두 유지 — 회귀 없음. - 성능: 추가 API 호출 없음. provisioner 문자열 비교 한 번뿐 — 회귀 없음.
- 에러 처리: no-provisioner 를 conclusive 로 둔 것은 events gap 하에서도 근거 있는 확정이라 정합합니다.
- 테스트: no-provisioner 두 모드, evidence 필드, events gap 하 유지, multiple-default 경로까지 덮여 충실합니다. 다만 finding 2 의 테스트 한 건은 사실상 항상 통과합니다.
1. src/korvid/core/pvc_analysis.py:464 — Severity: Warning
문제: 새 no-provisioner finding 이 _check_static_binding 이 이미 쓰고 있는 pvc.awaiting_static_volume rule_id 를 재사용해, 원인도 evidence 스키마도 다른 두 상태가 구조화 출력에서 구분 불가능해집니다.
return _finding(
"pvc.awaiting_static_volume",
"info",
"high",pvc_analysis.py:245 의 기존 finding 은 storageClassName: "" 인 경우로 evidence 가 spec.storageClassName (PVC 필드) 이고, 새 finding 은 StorageClass 가 존재하되 provisioner 가 없는 경우로 evidence 가 provisioner / volumeBindingMode (SC 필드) 입니다. rule_id 는 이 analyzer 의 안정적 계약 키이므로, rule_id 로 분기하는 소비자(에이전트 프롬프트, 대시보드, 회귀 테스트)는 어느 원인인지 알 수 없고 evidence 필드 존재 여부로 역추론해야 합니다. next_checks 도 서로 달라 잘못된 안내로 이어질 수 있습니다.
수정: 원인별로 별도 rule_id 를 부여하십시오.
return _finding(
"pvc.no_dynamic_provisioner",
"info",
"high",
pvc_id,
...
)(기존 explicit-empty 경로는 pvc.awaiting_static_volume 를 그대로 유지하면 하위 호환이 깨지지 않습니다. 두 id 모두 문서/프롬프트에 노출되지 않으므로 추가 갱신 부담도 없습니다.)
2. tests/core/test_pvc_analysis.py:761 — Severity: Suggestion
문제: 회귀를 막으려는 assertion 이 disjunction 때문에 실질적으로 항상 통과합니다.
assert "provisioner" not in next_checks_text or "static" in next_checks_text, (
f"Should not suggest provisioner logs: {report.findings[0].next_checks}"
)next_checks 에 "check provisioner pod logs" 가 다시 들어오더라도, 같은 튜플에 "static" 이 포함된 항목이 하나라도 있으면 통과합니다. 막으려던 회귀(pvc.provisioning_pending 로의 복귀)를 이 테스트만으로는 잡지 못합니다.
수정: 금지 문구를 직접 단정하고 rule_id 도 함께 고정하십시오.
assert report.findings[0].rule_id != "pvc.provisioning_pending"
assert not any("provisioner" in c.lower() and "log" in c.lower()
for c in report.findings[0].next_checks), report.findings[0].next_checks기존 리뷰에서 제기된 _pvc_storage_classes 스냅샷 생성 블록 중복(executor.py)과 StorageClassSummary invariant 부재(k8s/models.py)는 이번 커밋이 해당 블록을 건드리지 않아 재게시하지 않습니다.
Summary
pvc.bindingv1 rules for Bound/Lost, static and pre-bound claims, default/named StorageClasses, WaitForFirstConsumer, and provisioning-failure eventsdiagnose_pvcstructured-YAML tool to full/small agents and MCP, with PVC follow navigationWhy
PVC diagnosis was previously available only as a small text subsection under pod diagnosis. This phase makes PVC binding evidence deterministic and directly queryable without requiring a Pod or relying on model inference.
Read contract
Verification
make check: Ruff, mypy, pytest (4,410 passed, 21 skipped), tachScope
Implements the PVC/StorageClass phase of #191. PV topology, CSI/VolumeAttachment health, node/scheduling/quota, and missing-reference analyzers remain follow-up work.