fix: return complete pod scenario descriptions - #8222
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Root cause fixed at the parser, display truncation reuses the existing [DESIGN-REVIEWED] 3dd08fd |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've analyzed candidate 1 and the diff. Candidate 1 claims No other grounded defect surfaced in the changed lines: for the real short-named fixtures No findings. [OPUS-REVIEWED] 3dd08fd Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of Verification complete. First-Principles-Verdict: CONCERNS The fix is real and mechanism-level, but What this change shipsIntent: make
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 3dd08fd |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
9c5da03 to
03af8fd
Compare
|
|
|
03af8fd to
07c25ed
Compare
|
Amended head to What changed: the two tests here that run the real CLI against the packaged fixtures tree were asserting the entire registry — exact name list, positional row indexing ( Red-first evidence: copying a single fixture into Fix: the real-registry tests now name the three baseline fixtures explicitly, require them as a subset, key rows by name instead of position, and derive the expected hint line from the data. Why it belongs in this PR rather than a follow-up: this PR is already rewriting these exact assertions and was adding new positional ones ( Verification: green in both directions — 17 passed with an extra fixture present, 17 passed after removing it, 119 passed across |
07c25ed to
4a2d75b
Compare
|
|
The three test-shard failures on this PR are inherited from
all with Verified against pristine Root cause and fix are already identified upstream: the tests' (The |
Preserve complete manifest descriptions for JSON consumers and keep the human table sentence- and word-safe. Literal blocks retain their structure, folded blocks normalize whitespace, and the runtime remains dependency-free. Also decouple the two real-registry tests from the exact fixture list. They spawned the CLI against the packaged tree and then asserted the whole registry: an exact name list, positional row indexing, and a header whose padding is derived from the longest fixture name. Adding a fixture is a supported operation -- the registry is a directory scan -- so any addition reddened tests that are about how this command FORMATS its output, not about which fixtures exist. Verified by copying one fixture into the tree: two tests failed, the first on header padding. The real-registry tests now name the three baseline fixtures, require them as a subset, key rows by name, and derive the expected hint from the data. A new stubbed-registry test pins the tolerance property on an input the test controls, adding a name that sorts ahead of every baseline fixture and is longer than the header so it moves row order and column padding at once. No production behavior changes.
4a2d75b to
3dd08fd
Compare
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
bolichen97
left a comment
There was a problem hiding this comment.
fixture_summary() now reads the whole indented |/> block instead of returning the first physical line, so pod scenarios --json stops emitting descriptions cut mid-clause, and the human table delegates shortening to the existing tips_text.truncate_summary rather than a second implementation. Coverage matches the behavior change: literal-vs-folded newline handling, the malformed/empty degrade-to-empty path, and the word-boundary and long-token truncation cases are each pinned, and the real-registry assertions were reworked to key off fixture names so adding a fixture cannot redden a formatting test.
Return complete pod scenario descriptions
kirocrew pod scenarios(shipped in #8127) prints sentence fragments cut mid-clause, in both human and--jsonoutput:Root cause: each fixture manifest's
description:is a block scalar spanning several physical lines, andfixture_summary()returned the first physical line. An agent choosing a scenario from--jsontherefore received truncated data. Found by exercising the real CLI during the pod-api publication pass, not by a reviewer.Behavior change
--jsonnow carries the complete description. Literal (|) blocks preserve newlines and list structure; folded (>) blocks normalize whitespace into one paragraph.tips_text.truncate_summarymechanism rather than maintaining a second implementation.After:
Unchanged
PyYAML stays out of the runtime dependency set (the recorded #7841/#8127 decision; the test nulling
sys.modules["yaml"]still passes). Missing/empty/malformed descriptions still degrade to an empty field, never a failure. Empty-registry behavior, plain/quoted scalar support, exit codes, and fixture contents are untouched. Only the existingempty,minimal, andrichfixtures remain.Verification
Red-first adjudication: a literal-block structure test and the shared-helper long-token behavior both failed against the previous head, then passed with the fix. Focused seed/pod suites: 608 passed, 1 skipped; scenario suite: 16 passed. Black on the three touched Python files, isort, flake8, mypy
--platform linux(1,281 files), subprocess-encoding, and scoped black gates all pass. The three unrelated baseline-formatting hunks identified by First Principles were removed. Docs rows indocs/system-specs/modules/cli.mdandsrc/kiro_crew/pod/README.mdwere updated in the same commit.5 files, +169/−21.
Pattern harvest
Rule candidate: review-time checklist for hand-rolled parsers of a structured-format subset.
Pattern: a bespoke mini-parser consumes a multi-line construct but returns
after the first physical line. Here
fixture_summaryrecognised the|/>block markers and then returned the first non-empty indented line, so every
description was cut wherever the manifest author happened to wrap. The parser
was correct on the one-line
description: valueform, which is why tests andreview passed it.
The generalizable shape: when a parser deliberately implements a subset of a
format to avoid a dependency, each distinct syntactic form it claims to accept
needs its own case — and a multi-line form needs a fixture that actually spans
lines. A single-line fixture cannot distinguish "parses block scalars" from
"parses the first line of block scalars". This PR adds that coverage for
literal and folded blocks, including the literal-vs-folded newline
distinction Design review raised.