Skip to content

fix: return complete pod scenario descriptions - #8222

Merged
pepmach merged 1 commit into
mainfrom
feat/scenarios-desc
Sep 4, 2026
Merged

fix: return complete pod scenario descriptions#8222
pepmach merged 1 commit into
mainfrom
feat/scenarios-desc

Conversation

@pepmach

@pepmach pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Return complete pod scenario descriptions

kirocrew pod scenarios (shipped in #8127) prints sentence fragments cut mid-clause, in both human and --json output:

empty     A KIROCREW_HOME with nothing in it but this manifest. The gateway writes its
minimal   Populated KIROCREW_HOME with two workspaces, preferences/projects/semantic/

Root cause: each fixture manifest's description: is a block scalar spanning several physical lines, and fixture_summary() returned the first physical line. An agent choosing a scenario from --json therefore received truncated data. Found by exercising the real CLI during the pod-api publication pass, not by a reviewer.

Behavior change

  • --json now carries the complete description. Literal (|) blocks preserve newlines and list structure; folded (>) blocks normalize whitespace into one paragraph.
  • The human table normalizes the scalar for a single row, then shows the first complete sentence, shortened between words with an ellipsis when needed.
  • Word-boundary shortening reuses the existing tips_text.truncate_summary mechanism rather than maintaining a second implementation.

After:

SCENARIO  DESCRIPTION
empty     A KIROCREW_HOME with nothing in it but this manifest.
minimal   Populated KIROCREW_HOME with two workspaces, preferences/projects/semantic/ history…
rich      Superset of the ``minimal`` fixture that layers extra chat sessions, folder metadata, and…

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 existing empty, minimal, and rich fixtures 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 in docs/system-specs/modules/cli.md and src/kiro_crew/pod/README.md were 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_summary recognised 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: value form, which is why tests and
review 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.

@pepmach
pepmach requested a review from a team as a code owner September 3, 2026 17:50
@pepmach
pepmach requested a review from cixuuz September 3, 2026 17:50
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 3dd08fd112101b4122552c9286674606aa30c71f — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Root cause fixed at the parser, display truncation reuses the existing truncate_summary, PyYAML stays out — proportionate and in the right layers.

[DESIGN-REVIEWED] 3dd08fd

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 3dd08fd112101b4122552c9286674606aa30c71f — this comment is updated in place on each push.

Review details

I've analyzed candidate 1 and the diff.

Candidate 1 claims description_width can go zero/negative. Technically, with limit ≤ 0, truncate_summary misbehaves (a negative limit makes window = text[:limit+1] slice off only the tail, returning nearly the whole string). But this requires a fixture directory name ≥ 98 characters. available_fixtures() scans only the packaged kiro_crew/tests_fixtures/ tree (empty, minimal, rich — all short), which is developer-controlled and shipped in the wheel. There is no runtime path where a caller, user, or attacker supplies such a name. The triggering input (a) is a hypothetical "if a developer created a ≥98-char fixture name" — precisely the "could/if a caller were to" condition Step 1 requires me to drop on, and the candidate itself rates it low confidence with no attacker-reachable path. It does not clear 80+.

No other grounded defect surfaced in the changed lines: for the real short-named fixtures description_width is 90, " ".join(description.split()) collapses the multi-line scalars before truncation, and the JSON path emits the full scalar as designed.

No findings.

[OPUS-REVIEWED] 3dd08fd

Verdict parsed from the review's SHA-scoped output markers for commit 3dd08fd112101b4122552c9286674606aa30c71f.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 3dd08fd112101b4122552c9286674606aa30c71f: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 3dd08fd112101b4122552c9286674606aa30c71f — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Verification complete. truncate_summary reuse is real (tips_text.py:16, exact claimed semantics); the defect is real (all three fixture manifests use multi-line | blocks); fixture_summary has one consumer (pod/cli.py:944). The load-bearing find: frontmatter.py is the repo's declared single home for hand-rolled block-scalar resolution (parse_block_scalar_header + fold_block_scalar, format-agnostic, differentially pinned against PyYAML), and the new _description_block in seed.py is a second, lossier resolver of the same construct. Also, the patch touches 6 files while the description says 5 — the black-baseline prune and an unrelated reformat hunk ride along undeclared.

First-Principles-Verdict: CONCERNS

The fix is real and mechanism-level, but _description_block re-spells the block-scalar resolver frontmatter.py exists to keep in one place.

What this change ships

Intent: make pod scenarios return whole fixture descriptions instead of first-line fragments — a FIX.

  1. --json carries each fixture's complete description — justified (the reported defect).
  2. Human table shows the last full sentence that fits, ellipsis between words — justified, reuses tips_text.truncate_summary.
  3. Human rows gain a fixed 100-char width cap — justified; one consumer, convention-named constant.
  4. Literal | blocks keep newlines in JSON — justified (YAML rule).
  5. Folded > blocks become one normalized paragraph — duplicate-adjacent of frontmatter.fold_block_scalar.
  6. pod/cli.py leaves the black baseline; unrelated _prune reformat — rides along, undeclared.
  7. Docs rows in cli.md and pod README updated — justified (same-commit invariant).

Watch

  • seed._description_block (src/kiro_crew/seed.py:112) is the repo's second hand-rolled block-scalar resolver. frontmatter.py's docstring says "the scanner logic lives here exactly once" precisely because copies drifted — and this PR's bug was that drift class. Grepped fold_block_scalar|_description_block: 2 resolvers now. seed.py's block_markers frozenset is also the exact six-bare-indicator recognizer frontmatter.py:69-76 records consolidating away (chomping markers recognized, semantics ignored).
  • The description says "5 files"; the diff has 6. The sixth — the black-baseline prune plus the _prune reformat (pod/cli.py hunk at _audit() — is an undeclared rider; AGENTS.md welcomes it "in its own commit".

Subtractions

  • Replace _description_block and block_markers in src/kiro_crew/seed.py with frontmatter.parse_block_scalar_header + frontmatter.fold_block_scalar, keeping the folded-to-paragraph choice as one " ".join(value.split()) post-step.

[FIRST-PRINCIPLES-REVIEWED] 3dd08fd

@pepmach
pepmach enabled auto-merge (squash) September 3, 2026 18:21
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 3dd08fd112101b4122552c9286674606aa30c71f and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 3dd08fd

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 3dd08fd112101b4122552c9286674606aa30c71f: <one-sentence reason>

@pepmach
pepmach force-pushed the feat/scenarios-desc branch from 9c5da03 to 03af8fd Compare September 3, 2026 19:03
@pepmach

pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — preserve literal-block structure (span=5fef8fc3cdef)

    | descriptions now retain newlines and list indentation in JSON, while > descriptions remain folded. test_description_parser_preserves_a_literal_multiline_scalar failed on the prior head and passes now.

@pepmach

pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — remove unrelated baseline formatting (span=b353bdaf29da)

    The _prune, seed_cmd, and _safe_audit format-only hunks are absent from the final diff; only description behavior, tests, and matching docs remain.

@pepmach

pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — reuse the existing shortening helper (span=25c860f983b6)

    _scenario_table_description now calls tips_text.truncate_summary; the duplicate textwrap.shorten branch and degenerate-width guards were removed. test_human_output_reuses_shared_long_token_fallback failed on the prior head and passes now.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@pepmach
pepmach force-pushed the feat/scenarios-desc branch from 03af8fd to 07c25ed Compare September 3, 2026 19:24
@pepmach

pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Amended head to 07c25edda — added one in-scope change, no production behavior change.

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 (rows[0] assumed to be empty), and a header string whose padding is computed from the longest fixture name. The registry is a directory scan (seed.available_fixtures()), so adding a fixture is a supported operation, yet any addition reddened tests that exist to pin how this command formats its output.

Red-first evidence: copying a single fixture into src/kiro_crew/tests_fixtures/ on this branch's own head failed two tests, the first on padding rather than on the list:

assert lines[0] == "SCENARIO  DESCRIPTION"
E   AssertionError: assert 'SCENARIO        DESCRIPTION' == 'SCENARIO  DESCRIPTION'

assert [row["name"] for row in rows] == ["empty", "minimal", "rich"]
E   At index 0 diff: 'apps-installed' != 'empty'
2 failed, 14 passed

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. test_output_absorbs_an_added_fixture_without_moving_known_rows pins the tolerance property on a stubbed registry whose input the test controls — the added name sorts ahead of every baseline fixture and is longer than the SCENARIO header, so it moves row order and column padding in one case.

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 (rows[0]/rows[1]/rows[2]). Landing those and then removing them in the next PR is churn, and any separate PR would conflict in the same two tests. The four stubbed-registry tests this PR adds were already registry-independent; this extends the same discipline to the two that were not.

Verification: green in both directions — 17 passed with an extra fixture present, 17 passed after removing it, 119 passed across test_pod_scenarios_command.py + test_seed.py + test_pod_seed_scenarios.py. isort, black, flake8 clean on the touched file (it is in no baseline, so black-clean was required), and mypy --platform linux reports no issues across 1,281 source files. Every pre-existing test in the file was preserved — verified by diffing removed def lines, which is empty.

@pepmach
pepmach force-pushed the feat/scenarios-desc branch from 07c25ed to 4a2d75b Compare September 3, 2026 21:34
@pepmach

pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — drop the redundant first-sentence pass (span=25c860f983b6)

    Taken in full: the re.match(r".*?[.!?](?=\s|$)") step is gone from _scenario_table_description, which is now just truncate_summary(" ".join(description.split()), width). Sentence-boundary detection exists in one place again. This is the second round on this span — the first fix reused truncate_summary for the shortening but left a second spelling of its sentence cut beside it, which is fair.

    I verified the equivalence claim rather than taking it: truncate_summary cuts after the last complete sentence that fits, so on a description whose second sentence does not fit, its result is the first sentence — including the empty fixture, where the assertion in test_human_output_lists_sorted_names_and_first_sentences is unchanged and still passes. The one behavioural difference is the one the review named: a description whose second sentence fits now shows both sentences instead of hiding one. That is strictly more information in a column that had room for it, and it is still sentence-safe, so nothing regresses.

    The test was replaced, not deleted. test_human_output_prefers_first_complete_sentence asserted the behaviour being removed, so it could not stay — but deleting it outright would drop the sentence-safety coverage the Design fix added. Two tests replace it: one pins that a second sentence which fits is kept, and one pins the shipped shape (short first sentence, more prose than the column holds) still cutting at the sentence end rather than mid-word — which is precisely what makes the separate pass redundant.

    Docs updated in the same commit, since both asserted the removed behaviour: src/kiro_crew/pod/README.md and the pod scenarios row in docs/system-specs/modules/cli.md now describe the last complete sentence that fits, with a word-boundary ellipsis fallback.

    Two notes to pre-empt misreadings of the diff. The .github/black-baseline.txt deletion is mandatory, not churn: src/kiro_crew/pod/cli.py graduated to fully black-clean, and check_black_formatting.py FAILED with "1 graduated entry to prune" until it was removed. That is a file this PR already changes, so it is a different thing from the unrelated-file format hunks removed for this lane last round. And import re stays because _OLDER_THAN_RE still uses it.

    Validation. 64 passed in test_pod_scenarios_command.py + test_seed.py; 1387 passed / 1 skipped across the pod, seed, scenarios and tips suites. black (baseline pruned), subprocess-encoding, isort, flake8, mypy --platform linux (1281 files), docs-lint.sh (259 files) and the brand gate all clean. Diff is 6 files; no new flag, header, route or protocol, and no deferred scenario payload restored.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@pepmach

pepmach commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

The three test-shard failures on this PR are inherited from main, not from this change — evidence below so the red rollup doesn't read as this PR's defect.

Backend Tests (Windows) (1) and Backend Tests (3.12, 1) fail the same five tests on both platforms:

test_autonudge_stop_auth.py::test_applier_owed_terminal_turn_is_not_reported_as_a_spent_cap
test_autonudge_stop_auth.py::test_applier_settled_terminal_loop_is_not_reported_as_a_manual_pause
test_autonudge_stop_auth.py::test_applier_owed_blocked_turn_is_not_reported_as_a_merge
test_autonudge_stop_auth.py::test_applier_a_settled_outcome_outranks_a_stale_owed_turn
test_autonudge_stop_auth.py::test_applier_a_spent_cap_with_no_terminal_news_still_revives

all with AssertionError: assert 'merged' in 'monitor_update cannot apply legacy fields to a structured monitor: …'.

Verified against pristine origin/main (5df44de2f, no branch changes): the identical five tests fail there — 5 failed, 27 passed in test_autonudge_stop_auth.py. This PR touches pod scenarios description parsing only (seed.py, pod/cli.py, docs, and their tests); it contains no monitor, autonudge, or dashboard-directive code.

Root cause and fix are already identified upstream: the tests' _FakeLoop predates the structured-monitor classifier, so they exercise a branch is_structured_monitor_loop now makes unreachable — pinned by PR #8321, which touches only test/test_autonudge_stop_auth.py. Once that merges, a rerun of the failed shards here should clear them; no change to this PR is warranted.

(The Coverage Gate red is the cascade of the failed shards; the Dependency Audit red is a separate, intermittent npm audit 120s timeout tracked with the repository owner.)

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.
@pepmach
pepmach force-pushed the feat/scenarios-desc branch from 4a2d75b to 3dd08fd Compare September 4, 2026 05:23
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This 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

  • PR #8218 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8218: KEEP. Independent, complementary changes to the same file set. Keep both; the second to merge resolves the cli.md verb-table conflict and drops the now-redundant black-baseline removal. Files: docs/system-specs/modules/cli.md, src/kiro_crew/pod/cli.py.
  • This PR is OVERLAPPING with PR #8127. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8222: KEEP. PR #8127 is the merged predecessor that created the defect; nothing in it or in current main covers the fix. Files: src/kiro_crew/seed.py, src/kiro_crew/pod/cli.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

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.

2 participants