Small-model agent UX: follow mode, composite-name guards, prompt consolidation - #172
Conversation
Prompt text was split across runtime.py (full-profile role statement, write/no-write/UI-drive clauses, composition) and profiles.py (small variants importing the full ones back from runtime). Prompt wording is policy, the runtime is mechanism: one module now owns every string and compose_system_prompt; profiles.py keeps budgets and surface selection. Also sharpens both role statements against observed small-model failures (404 loops): tools-only boundary (no shell/kubectl), list-before-inspect, name/namespace pairing, and 404-means-relist recovery, pinned by new assertions in tests/agent/test_profiles.py.
Small local models rarely volunteer the UI tools (open_describe, open_logs): they call the data-returning cluster reads and answer in text while the screen sits idle. With agent follow on (the default), each successful read in a chat turn is mirrored through the same UIBridge mapping MCP follow mode (issue #153) uses for external reads — list_resources navigates, get_resource/get_events/diagnose_pod open describe, get_logs opens the log pane. Failed reads (404) never steer the screen; broken tool-call JSON is skipped, never raised; the existing bridge guards (approval dialogs, screens the user is reading) refuse rather than cover. Config: agent.follow (only a literal false disables); runtime toggle: :ai follow [on|off].
Observed with small local models: the screen context's selected= field
carried the raw row key ('default/otel-collector-...'), which the model
pasted verbatim as a resource name and paired with whatever namespace
the user mentioned — a guaranteed 404 and a burned loop iteration.
Two layers:
- _screen_context now splits the composite into selected= (bare name)
and selected_ns=, handing the model the two fields tool calls take.
- The read tools (get_resource, get_events, get_logs, diagnose_pod)
reject slash-containing names locally — Kubernetes names can never
contain '/' — with wording that teaches the split, before any API
round-trip.
There was a problem hiding this comment.
🟡 Not ready to approve
The screen-context regression test can pass without exercising its intended behavior, and user-facing follow-mode documentation is incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Improves small-model agent reliability and visibility.
Changes:
- Consolidates prompt policy in
agent/prompts.py. - Adds default-on agent follow mode and runtime toggle.
- Splits composite row keys and rejects slash-containing resource names.
File summaries
| File | Description |
|---|---|
src/korvid/agent/prompts.py |
Centralizes prompt wording. |
src/korvid/agent/profiles.py |
Uses centralized profile prompts. |
src/korvid/agent/runtime.py |
Delegates prompt composition. |
src/korvid/core/config.py |
Adds agent.follow configuration. |
src/korvid/tools/executor.py |
Rejects composite resource names. |
src/korvid/ui/app.py |
Implements follow mode and context splitting. |
tests/agent/test_profiles.py |
Verifies prompt invariants. |
tests/agent/test_runtime.py |
Updates prompt imports. |
tests/core/test_config.py |
Tests follow configuration. |
tests/tools/test_executor.py |
Tests slash guards. |
tests/tools/test_write_tools.py |
Updates prompt imports. |
tests/ui/test_agent_follow.py |
Tests follow behavior. |
tests/ui/test_agent_wiring.py |
Tests selected-resource context. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 4
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…tring accuracy Addresses Copilot review round 1 on #172: - the screen-context split test now waits (tests/ui/waits.py::until) for the watch to land the row, then asserts the exact selected=web-1 / selected_ns=default tokens — it can no longer pass on an empty table - _reject_slash_name docstring no longer claims the rejection saves a loop iteration (it saves the API round-trip and improves guidance) - profiles.py module docstring no longer claims full reproduces the pre-profile wiring byte-for-byte (this PR changed its prompt wording) - docs/agent.md documents follow mode: default-on, YAML disable, and the :ai follow runtime toggle
There was a problem hiding this comment.
🟡 Not ready to approve
Follow mirroring can still override an active describe screen despite the documented user-priority guard.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
src/korvid/ui/app.py:8456
- The new follow path does not uphold the stated “screens the user is reading refuse the mirror” guard.
agent_open_describe()does not reject an already-openDescribeScreen(only navigation does), so if the user hides chat and opens a describe modal while a turn is running, a successfulget_resource,get_events, ordiagnose_podcan push another describe over it; this also contradictsdocs/agent.md:207-209. Refuse follow mirroring while aDescribeScreenis active and add a regression test for that in-flight-turn scenario.
await mirror_read(AppUIBridge(self), name, arguments)
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
소형 모델 UX 3종 세트(프롬프트 통합 / agent follow / composite-name 가드) 리뷰했습니다. 전반적으로 견고합니다.
좋았던 점
agent/prompts.py로의 프롬프트 통합이 순수한 1:1 이동 + 강화(문구는 policy, runtime은 mechanism)로 깔끔하고,profiles.py의 역수입(runtime→profiles) 의존이 제거됨.compose_system_prompt는 armed tool set 조건부 절 구성 로직까지 그대로 보존.- agent follow가 MCP follow(#153)와 동일한
mirror_read/FOLLOWABLE_TOOLS경로를 재사용 — #153에서 이미 잡힌 diagnose_podpod스키마 매핑, 승인 다이얼로그/읽는 중 화면 거부 가드를 공짜로 상속. 실패한 읽기(ok=False)·깨진 JSON은 미러하지 않는 처리와 테스트 모두 확인. _reject_slash_name슬래시 가드가 API 왕복 전에 로컬 거부 + 분리 방법을 가르치는 에러 문구,_ExplodingKube로 "클러스터에 절대 안 닿음"을 증명하는 테스트 설계가 좋음._screen_context의 selected/selected_ns 분리 +selected=default/web-1부재 단언, configfollow: banana → True(literal false만 off) 테스트 등 엣지 커버 충실.
Suggestion (비차단)
_reject_slash_name이 name/pod 필드에만 적용되고namespace필드는 여전히 무가드입니다. 모델이 반대로 namespace에default/web-1을 넣는 변형도 관찰될 수 있으니namespace = _reject_slash_name(str(args["namespace"]), "namespace")로 대칭 적용을 고려하세요 (get_logs/get_events/diagnose_pod/get_resource 4곳).- MCP follow는 거부된 미러를 activity note로 강등해 가시성을 남기지만(
_mirror_or_note, run-cc), agent follow는 브리지가 미러를 거부하면 조용히 사라집니다. 에이전트 패널에 읽기 자체는 보이므로 수용 가능하지만, 두 follow 모드의 강등 정책이 비대칭이라는 점만 기록해 둡니다. _maybe_follow_agent_read가 turn 이벤트 루프 안에서mirror_read를 직접 await 합니다 — MCP follow의 fire-and-forget과 달리 느린 미러(예: open_logs의 pod lookup)가 다음 이벤트 처리를 지연시킬 수 있습니다. 실측상 문제 없으면 그대로 두어도 됩니다.
APPROVE
Post-review hardening on #172: - round 2's suppressed finding was credible: agent_open_describe only guarded approval dialogs, so a follow mirror (or agent describe) landing while the user reads a DescribeScreen covered it - breaking the docs/agent.md contract ('a mirror is refused while … a describe screen you are reading is open'). _describe_precheck now refuses like agent_navigate/agent_drill_down; the panel-visible path (non- modal describe pane) is unaffected (test_mirror_refuses_to_cover_a_describe_screen_the_user_is_reading). - reviewer suggestion applied: the slash guard now also covers the namespace field on all four read tools - models paste the composite in either direction, and namespace names can never contain '/' either (test_slash_in_namespace_is_rejected_symmetrically, 4x parametrized against the exploding kube). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
리뷰 잔여 사항 2건을 ddbf665에서 처리했습니다:
제안 #2(강등 정책 비대칭)와 #3(inline await)은 기록대로 수용 — 에이전트 패널에 읽기가 이미 보이므로 별도 조치 없음. Full gate 그린 (3033 passed). |
There was a problem hiding this comment.
🟡 Not ready to approve
Agent-follow mirrors bypass the shared UI serialization lock, allowing unsafe overlap with concurrent MCP UI operations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
tests/agent/test_profiles.py:130
- These independent token checks do not actually pin the advertised tools-only invariant: deleting the sentence “You explore the cluster only through the tools provided…” still leaves both
only(in “use only names”) andtoolselsewhere, so the test remains green. Assert the defining phrases together (and likewise the concrete pairing/re-list recovery wording) so future prompt edits cannot silently remove the policy this test is meant to protect.
prompt = SYSTEM_PROMPT.lower()
assert "only" in prompt
assert "tools" in prompt
assert "no shell" in prompt
assert "never invent" in prompt
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Review finding on #172: _maybe_follow_agent_read built a fresh AppUIBridge, bypassing the composition root's _UIBridgeProxy lock that exists precisely because UI operations (log-pane swaps, describes) are not safe to interleave between the built-in agent and concurrent MCP calls. The app now holds the injected shared proxy (_agent_follow_bridge, wired at the same site as ui_proxy.target) and mirrors through it; a missing injection (tests, degraded wiring) falls back to a direct adapter. Regression: test_mirror_routes_through_the_injected_serialized_bridge - a mirror issued while the proxy's lock is held (an in-flight MCP UI call) queues behind it instead of interleaving, and lands after release. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review suppressed finding on #172: 'only' + 'tools' as independent token checks stayed green even with the tools-only sentence deleted. Both invariant tests now assert the defining clauses ('only through the provided tools', 'call list_resources first', 'name and namespace appear together', 're-list instead of retrying', …) so removing the behavior-bearing wording fails the test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The small-model prompt still reinforces composite-name misuse, and the new wiring and timing test violate explicit repository conventions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
src/korvid/agent/prompts.py:86
- The new grounding rule conflicts with both the following exemplar and the actual
list_resourcesformat. It permits only pairs from tool results, but the example immediately trusts the pair from the user; moreover,list_resourcesemitsnamespace/name(src/korvid/tools/executor.py:655), so “copy names exactly” reinforces the composite-name failure this PR is meant to prevent. Allow explicitly paired current-user/screen-context values, and tell the model to split list rows into the separate namespace and name fields.
"next step. Never invent resource names or namespaces: unless the exact "
"name and namespace appear together in a tool result of this "
"conversation, call list_resources first and copy names exactly from "
"its output. A 404/NotFound means the name or namespace is wrong — "
tests/ui/test_agent_follow.py:163
- This fixed 50 ms delay can pass before the turn has actually reached the locked bridge, so the negative assertion does not prove that the mirror queued. The repository explicitly requires condition polling for Textual tests (
AGENTS.md:123-124,tests/ui/waits.py:1-4); wait until the lock has a waiter (or expose an event from a test bridge) before asserting the screen is unchanged.
await pilot.pause(0.05)
src/korvid/main.py:928
- This wires a new dependency by mutating a private app attribute after construction, contrary to the repository rule that dependencies are constructor-injected and wired once in
__main__.py(AGENTS.md:34,55). The proxy already exists beforeKorvidAppis created, so pass it as anagent_follow_bridgeconstructor argument and initialize_agent_follow_bridgefrom that parameter instead.
app._agent_follow_bridge = ui_proxy
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…test Review round on #172 (all three suppressed findings applied): - agent_follow_bridge is a constructor parameter wired at KorvidApp construction (AGENTS.md: constructor injection, wired once) - the post-construction attribute mutation is gone; ui_proxy exists before the app, so nothing needed late binding. - the small prompt's grounding rule no longer reinforces the composite bug it fights: list_resources rows start with 'namespace/name', so 'copy names exactly' taught exactly the wrong move, and the rule contradicted the exemplar's user-given pair. New wording allows user/screen-context pairs and tells the model to split list rows into the separate fields; the invariant tests pin the new clauses. - the serialization regression test polls until the mirror is really blocked on the proxy lock (AGENTS.md: no wall-clock waits) instead of a fixed 50ms pause that could pass vacuously. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
이번 라운드의 suppressed 3건 모두 적용했습니다:
Full gate 그린 (3034 passed). |
There was a problem hiding this comment.
🟡 Not ready to approve
Log mirroring can still alter streams beneath an active describe screen, violating the documented user-priority guard.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/korvid/ui/app.py:8463
get_logsdoes not honor the documented “refuse while a describe screen is open” rule. This call mapsget_logstoagent_open_logs, which only guards approval dialogs and will cancel/open log streams beneath an activeDescribeScreen; the new regression covers onlyget_resource, which takes_describe_precheck. Add a describe-screen guard for log mirrors at the serialized bridge/app boundary and cover this mapping in the follow test.
await mirror_read(self._agent_follow_bridge or AppUIBridge(self), name, arguments)
tests/ui/test_agent_follow.py:55
- Add a reason for this suppression;
AGENTS.md:63requires every codedtype: ignoreto include an explanatory comment, as the other fakes in this file do.
app._agent_runtime = _ScriptedRuntime(_read_events(ok=False)) # type: ignore[assignment]
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
재리뷰 — 신규 커밋 4건 (6fbed56 → c737dbf). 이전 리뷰의 지적/제안 사항이 모두 해소되었습니다.
해결 확인:
- namespace 슬래시 가드 대칭화 —
_reject_slash_name이 4개 read tool(get_resource/get_logs/get_events/diagnose_pod)의 namespace 필드에도 적용._ExplodingKube기반 4-way parametrized 테스트로 API 호출 전 로컬 거부 입증. - follow 미러의 공유 직렬화 브리지 경유 —
_maybe_follow_agent_read가 freshAppUIBridge대신 composition root의_UIBridgeProxy(agent_follow_bridge생성자 파라미터,__main__에서ui_proxy주입)를 사용. MCP UI 호출과의 interleaving 차단. 락 점유 중 미러가 실제로 대기열에 걸렸는지until()폴링으로 확인하는 비-vacuous 회귀 테스트 (고정 sleep 없음). - describe 화면 커버 방지 —
_describe_precheck가 approval dialog에 더해 사용자가 읽는 중인DescribeScreen도 거부, docs/agent.md follow 계약과 일치. 미러가 사용자 화면을 덮지 않는 회귀 테스트 포함. - 프롬프트 불변식 강화 — 토큰 단위(
"only","tools") 검사에서 정의 구절(defining clause) 검사로 교체 — 문장 삭제 시 실제로 테스트가 깨짐. small 프롬프트의 'copy names exactly' 문구가 composite 버그를 오히려 강화하던 문제도 수정 (row를 두 필드로 split하라는 명시적 지시로 대체).
Suggestion (advisory): 미러가 이제 프록시 락을 인라인으로 await하므로, 장시간 점유되는 MCP UI 호출 뒤에서 agent turn 완료가 지연될 수 있습니다 (기존 인라인-await 지적의 연장선 — 필요 시 timeout 또는 활동 노트 degrade 고려). 테스트가 proxy._lock/_waiters 내부에 의존하는 점도 사소한 결합이나 허용 범위입니다.
APPROVE
Review round on #172 (suppressed, credible): the get_logs mapping reaches agent_open_logs, which only guarded approval dialogs - a log mirror could cancel/swap the streams beneath the DescribeScreen the user is reading, violating the documented user-priority rule that describe/navigate/drill already honor. Guard added; the follow test now covers the get_logs mapping too (test_log_mirror_refuses_while_a_describe_screen_is_open). Also adds the missing reason comment on one type: ignore. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
이번 라운드 suppressed 2건 모두 적용:
두 라운드 연속 suppressed-only이며 이번 건까지 모두 수정 완료 — 리뷰 루프 정책에 따라 체크 그린 확인 후 머지 진행합니다. Full gate 그린 (3035 passed). |
There was a problem hiding this comment.
재리뷰 (신규 커밋 1개, c737dbf → f4b7b57) — APPROVE.
이전 라운드에서 언급된 잔여 지점을 정확히 닫았습니다: agent_open_logs가 approval dialog만 가드하고 DescribeScreen은 가드하지 않아, 사용자가 describe 모달을 읽는 중에 로그 미러가 그 아래 스트림을 교체/취소할 수 있던 user-priority 규칙 위반을 isinstance(self.screen, DescribeScreen) 가드로 차단했습니다. describe/navigate/drill과 동일한 규칙이 이제 get_logs 매핑에도 대칭 적용됩니다.
테스트도 실질적입니다: test_log_mirror_refuses_while_a_describe_screen_is_open가 실제로 DescribeScreen을 띄운 뒤 get_logs 턴을 돌리고, app.screen is reading (모달 유지) + not app._log_pane.display (스트림 미개방) 두 축을 모두 단언 — vacuous하지 않습니다. type: ignore에 사유 주석 추가도 확인.
지적 사항 없음.
Summary
Improves the built-in agent's UX with small local models (3B-8B over Ollama), where three failure modes were observed in live testing against a real cluster:
open_describe,open_logs).namespace/namecomposites (from the screen context'sselected=field) as resource names and pairs them with the wrong namespace — a guaranteed 404 and a burned loop iteration.runtime.pyandprofiles.py, withprofilesimporting the full-profile wording back out of the runtime.Commits
1.
refactor(agent): consolidate all prompt wording intoagent/prompts.pyPrompt wording is policy, the runtime is mechanism. One module now owns every role statement, clause, small-profile tool-description override, and
compose_system_prompt;runtime.pykeeps loop mechanics,profiles.pykeeps budgets and surface selection (its reverse import of full-profile prompts is gone).Both role statements are also sharpened against the observed 404 loops: tools-only boundary (no shell/kubectl), list-before-inspect, name/namespace pairing, and 404-means-relist recovery — each pinned by prompt-invariant assertions.
2.
feat(agent): follow mode — mirror the agent's cluster reads on screenWith agent follow on (the default), each successful cluster read in a chat turn is mirrored through the same
UIBridgemapping MCP follow mode (#153) uses for external reads:list_resourcesnavigates,get_resource/get_events/diagnose_podopen describe,get_logsopens the log pane.agent.follow(only a literalfalsedisables); runtime toggle:ai follow [on|off].3.
fix(agent): stop feedingnamespace/namecomposites to the modelTwo layers against failure mode 2:
_screen_contextsplits the raw row key intoselected=(bare name) andselected_ns=, handing the model the two fields tool calls actually take.get_resource,get_events,get_logs,diagnose_pod) reject slash-containing names locally — Kubernetes names can never contain/— with wording that teaches the split, before any API round-trip.Testing
tests/ui/test_agent_follow.py(7 tests: mirror/404-refusal/toggle/broken-JSON/config default), slash-guard parametrized tests (guarded by an exploding fake kube proving no API call), screen-context split test, prompt-invariant assertions,agent.followconfig parsing.main.Related