Skip to content

fix: list_resources carries the TUI's status columns — no more N+1 get_resource - #162

Merged
hellices merged 4 commits into
mainfrom
fix/list-resources-columns
Aug 2, 2026
Merged

fix: list_resources carries the TUI's status columns — no more N+1 get_resource#162
hellices merged 4 commits into
mainfrom
fix/list-resources-columns

Conversation

@hellices

@hellices hellices commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Closes #158

Problem

_list_resources rendered one shape for every kind — namespace/name - age=… — dropping every status fact the summaries carry. The observed cost: an MCP host asked to "check pod status" got status-less rows, then issued N+1 get_resource calls (full masked manifests, token-expensive) to reconstruct what the TUI table already showed; worse, on the pods path the data was dropped twice (summary_for("Pod") fell through to GenericSummary, capturing nothing), so the host could confidently report pods "fine" while the screen showed CrashLoopBackOff.

Fix

1. Pod-aware LIST summarysummary_for dispatches Pod to a new PodListSummary(GenericSummary): kubectl-style display STATUS (waiting reasons like CrashLoopBackOff included, via the existing _display_phase), READY n/m, RESTARTS, NODE. Tool-path only: the watch path's richer PodSummary and the resource store are untouched.

2. Kind-aware facts, type-keyedlist_resources lines now mirror the TUI columns per kind:

Kind Line facts
pods phase=… ready=n/m restarts=k node=…
replicasets revision=… desired=… current=… ready=…
subscriptions / CSVs / packagemanifests the same facts list_operators prints
generic kinds desired=n when spec.replicas exists
helm release/revision renderers registered ahead of the listing tool (#161)

3. Custom columns (issue #45) — user-configured columns reach the model as NAME=value (clamped to 80 chars/value so a hostile value cannot dominate the result budget). ToolExecutor takes the configured column names per plural — same source as KubeClient's value wiring — injected at all three construction sites in __main__.py.

4. Drift contract — the renderer table is the contract: test_every_typed_summary_has_a_facts_renderer walks GenericSummary.__subclasses__() and fails when any typed summary lacks a facts renderer, so a future kind cannot silently degrade back to name+age. (It already caught HelmReleaseSummary/HelmRevisionSummary during development.)

Split out: #161 (helm_list_releases read tool — needs a new MCP-surface tool + follow-mode pairing, its own review cycle).

Testing

tests/tools/test_list_resources.py (new, 10 tests): Pod summary capture incl. CrashLoopBackOff-as-status, per-kind facts lines, generic desired/empty cases, the subclass contract, end-to-end tool output for pods and custom columns (incl. clamping). Wiring pinned in tests/test_main_wiring.py::test_mcp_executor_receives_custom_column_names.

Full gate green: ruff, mypy --strict, tach, 2966 passed / 21 skipped, coverage ≥ 80%.

The tool rendered one shape for every kind - namespace/name + age -
dropping everything else the summaries carry. An MCP host asking about
pod status got rows with no status, then burned N+1 get_resource calls
(full masked manifests) to reconstruct what the table already showed;
worst case the host confidently reported pods 'fine' while the screen
showed CrashLoopBackOff.

- summary_for dispatches Pod to a new PodListSummary(GenericSummary):
  STATUS (kubectl-style display phase incl. waiting reasons) / READY /
  RESTARTS / NODE, reusing _display_phase. Tool-path only - the watch
  path's PodSummary and the store are untouched.
- list_resources renders kind-aware facts via a type-keyed renderer
  table: pods phase/ready/restarts/node, replicasets revision/desired/
  current/ready, OLM kinds the same facts list_operators prints,
  generic desired=n when present. Helm release/revision renderers are
  registered ahead of the listing tool (#161).
- user-configured custom columns (issue #45) reach the model as
  name=value, clamped to 80 chars per value; ToolExecutor takes the
  configured names, wired from the composition root at all three
  construction sites.
- the renderer table is the drift contract:
  test_every_typed_summary_has_a_facts_renderer fails when a future
  GenericSummary subclass has no facts renderer.

Closes #158

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 2, 2026 18:06

Copilot AI 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.

Pull request overview

Adds status-rich, kind-aware list_resources output to avoid N+1 manifest fetches.

Changes:

  • Adds pod and typed-summary fact rendering.
  • Exposes configured custom columns with value clamping.
  • Wires column names into agent and MCP executors with tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/korvid/tools/executor.py Renders kind-specific and custom facts.
src/korvid/k8s/models.py Adds pod-aware list summaries.
src/korvid/__main__.py Wires custom-column names.
tests/tools/test_list_resources.py Tests summary and output behavior.
tests/test_main_wiring.py Tests MCP column wiring.
Suppressed comments (2)

src/korvid/tools/executor.py:104

  • PackageManifestSummary.description is rendered in the TUI (resource_table.py:750-756) but is omitted here, so list_resources(kind="packagemanifests") still violates this PR's column-parity goal. Include a clamped description when present and assert it in the package facts test.
def _package_facts(s: PackageManifestSummary) -> str:
    return (
        f"catalog={s.catalog or '?'} default_channel={s.default_channel or '?'}"
        f" channels={_clamp(','.join(s.channels)) or '?'}"
    )

src/korvid/tools/executor.py:123

  • The Helm revision TUI includes APP VERSION (resource_table.py:709-716), but this registered renderer drops s.app_version. When the follow-up exposes this renderer, revision output will already drift from the screen; add the clamped field and cover the Helm renderers in the contract tests.
        f"chart={_clamp(s.chart)}",

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/korvid/tools/executor.py Outdated

@my-reviewer-agent my-reviewer-agent 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.

이슈 #158 해결 확인 — pods LIST 경로가 GenericSummary로 떨어져 상태가 두 번 사라지던 문제를 PodListSummary(기존 _display_phase 재사용, TUI PodSummary.from_manifest와 phase/ready/restarts/node 계산 로직 동일함을 head에서 대조 확인)로 닫았고, kind별 facts 렌더러 + __subclasses__() 드리프트 계약 테스트, 커스텀 컬럼 name=value(80자 클램프) 배선(main.py 3개 구성 지점 모두 확인)까지 설계가 깔끔합니다. CrashLoopBackOff-as-status 테스트가 이슈의 헤드라인 실패를 정확히 고정합니다. 인라인 2건은 비차단(개행 주입 Warning 1건, 계약 테스트 사각 Suggestion 1건)입니다.

APPROVE

Comment thread src/korvid/tools/executor.py
Comment thread tests/tools/test_list_resources.py Outdated
Review round 1 on #162: custom-column values come from arbitrary
annotations/JSONPath - length clamping alone still let embedded
newlines/control characters forge extra rows in the model-facing
result. _clamp now flattens non-printables to spaces before bounding
(same approach as #156's toast sanitizer); every fact path routes
through it (test_custom_column_values_cannot_forge_extra_rows).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@my-reviewer-agent my-reviewer-agent 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.

재리뷰 (신규 커밋 1건: 47e96bb)

라운드 1의 Warning(커스텀 컬럼 값에 포함된 개행/제어문자로 model-facing 결과에 가짜 행 주입 가능)을 정확히 해결했습니다.

  • _clamp가 길이 제한 전에 isprintable() 기반으로 비출력 문자를 공백으로 평탄화 — #156의 toast sanitizer와 동일한 접근이고, 모든 fact 경로(_pod_facts/generic/custom)가 _clamp를 경유하므로 커버리지 완전.
  • 회귀 테스트 test_custom_column_values_cannot_forge_extra_rows가 실제 공격 형태(개행으로 위조한 pod 행 + ANSI \x1b[2J)를 사용하고 splitlines()==1 + \x1b 부재를 단언 — 형식적이지 않은 진짜 테스트입니다.

지적사항 없음. 라운드 1의 __subclasses__() 직계-한정 Suggestion은 여전히 열려 있으나 advisory 수준이므로 승인에 영향 없습니다.

APPROVE

Copilot AI 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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/korvid/tools/executor.py:101

  • version and phase are raw CSV CRD strings and remain unsanitized, unlike display_name. A crafted value can therefore add fake result rows or dominate the capped output; clamp every cluster-derived string in this renderer.
    parts = [f"version={s.version or '?'}", f"phase={s.phase or '?'}"]
    if s.display_name:
        parts.append(f"display={_clamp(s.display_name)}")

src/korvid/tools/executor.py:95

  • These Subscription fields are cluster-controlled CRD strings, but none passes through _clamp. In particular, a user-supplied channel can contain an embedded newline or be arbitrarily long, reintroducing forged rows/result-budget exhaustion on this typed path.
    return (
        f"channel={s.channel or '?'} source={s.source or '?'}"
        f" csv={s.installed_csv or '?'} state={s.state or '?'}"
    )

src/korvid/tools/executor.py:109

  • catalog and default_channel are also cluster-derived strings, but only channels is clamped. Catalog data containing controls/newlines or a very long value can still forge rows or consume the result budget.
    return (
        f"catalog={s.catalog or '?'} default_channel={s.default_channel or '?'}"
        f" channels={_clamp(','.join(s.channels)) or '?'}"
    )

src/korvid/tools/executor.py:88

  • revision is read from the freely writable deployment.kubernetes.io/revision annotation, so it can contain newlines/control characters or an oversized value. Because it is emitted without _clamp, one ReplicaSet can forge extra rows or crowd out the real listing despite the new sanitization contract.
    return f"revision={s.revision} desired={s.desired} current={s.current} ready={s.ready}"

src/korvid/tools/executor.py:81

  • phase comes from pod status/waiting-reason strings and bypasses _clamp, so a malformed or hostile status can still inject a newline/control sequence into this model-facing list or consume the result budget. Apply the same single-line bounded rendering used for node and custom columns.

This issue also appears in the following locations of the same file:

  • line 88
  • line 92
  • line 99
  • line 106
    parts = [f"phase={s.phase or '?'}", f"ready={s.ready or '?'}", f"restarts={s.restarts}"]

Review round 2 on #162:

- length clamping had only been applied selectively: phase (pod status/
  waiting reasons), revision (a freely writable annotation), channel/
  source/csv/state, catalog/default_channel, version/phase and the helm
  status strings are all cluster-controlled - every renderer now routes
  every derived string through _clamp (flatten + bound)
  (test_every_cluster_derived_string_is_flattened).
- summary_facts dispatched on exact type(s): a grandchild subclass of a
  typed summary passed the direct-subclass contract test yet silently
  degraded to generic facts. Dispatch walks the MRO, and the contract
  test walks the subclass tree recursively
  (test_grandchild_summary_uses_its_parents_renderer).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI 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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/korvid/tools/executor.py:137

  • _helm_revision_facts drops app_version, even though the Helm revision table includes APP VERSION (resource_table.py:57,714) and the #161 contract names it as part of this renderer. Once the follow-up exposes this renderer, revision results will still disagree with the TUI. Add the clamped app version alongside the chart.
        f"chart={_clamp(s.chart)}",

Review round 3 on #162 (suppressed): the Helm revision table shows APP
VERSION - the renderer #161 will expose must agree with the TUI from
day one (test_helm_revision_facts_include_app_version).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@hellices

hellices commented Aug 2, 2026

Copy link
Copy Markdown
Owner Author

Round 3's suppressed finding is fixed: _helm_revision_facts now includes the clamped app_version alongside chart, matching the TUI's revision table (test_helm_revision_facts_include_app_version). Full gate green (2970 passed).

@my-reviewer-agent my-reviewer-agent 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.

재리뷰 (신규 커밋 2건) — 이전 라운드의 두 가지 잔여 사항이 모두 해결되었습니다.

  1. 클러스터 유래 문자열 전면 클램프: phase/revision(자유 기록 가능한 annotation)/channel/source/csv/state/catalog/default_channel/version/helm status 등 모든 렌더러가 _clamp(비인쇄문자 평탄화 + 길이 제한)를 경유하도록 확장. test_every_cluster_derived_string_is_flattened가 개행 주입 페이로드로 5개 summary 타입을 실검증 — 이전 라운드의 부분 적용 문제를 완전히 닫음.
  2. MRO 기반 renderer dispatch: 이전에 남긴 __subclasses__() direct-only Suggestion 해결. summary_factstype(s).__mro__를 순회해 손자 서브클래스도 부모 렌더러를 상속하고, contract 테스트도 서브클래스 트리를 재귀 순회(test_grandchild_summary_uses_its_parents_renderer)로 검증.
  3. helm revision facts에 app_version 추가: TUI의 APP VERSION 컬럼과 1:1 일치, 테스트 포함.

지적사항 없음. APPROVE

Copilot AI 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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@hellices
hellices merged commit 671c6cf into main Aug 2, 2026
10 checks passed
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.

list_resources drops every status column the TUI shows (name+age only) — forces N+1 get_resource calls from MCP/agent

2 participants