Add structured diagnostics and validation clarity#21
Open
bma-d wants to merge 56 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds diagnostics datatypes and redaction, parse/state/agent-plan validators and loaders, session-state diagnostics and monitor emission, CLI/tmux wiring with legacy-field preservation, agent-config frontmatter parsing, orchestrator refactors, and expanded tests/docs. ChangesObservability and Validation Infrastructure
Estimated code review effort: 🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
skills/bmad-story-automator/data/tmux-commands.md (1)
39-70: ⚡ Quick winPrefer helper-based
project-slug/project-hashcommands over repeated inline hash scripts.Line 39-Line 70 duplicates slug/hash derivation logic in multiple snippets. Using helper subcommands in docs reduces drift when naming rules evolve.
Suggested doc patch
-project_slug=$(basename "$PWD" | tr '[:upper:]' '[:lower:]' | tr -cd '[:alnum:]' | cut -c1-8) -project_hash=$(python3 - <<'PY' -import hashlib, pathlib -print(hashlib.md5(str(pathlib.Path.cwd().resolve()).encode(), usedforsecurity=False).hexdigest()[:8]) -PY -) +project_slug=$("$scripts" tmux-wrapper project-slug) +project_hash=$("$scripts" tmux-wrapper project-hash)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/bmad-story-automator/data/tmux-commands.md` around lines 39 - 70, Replace the repeated inline derivation blocks that set project_slug, project_hash, and session_name with calls to small helper commands or shell functions (e.g., a get_project_slug and get_project_hash helper or single sa-project-id command) and update every snippet that currently contains the Python/hash and basename pipelines (the blocks that assign project_slug and project_hash and the session_name usage) to invoke those helpers instead; ensure the tmux examples (the tmux list-sessions grep and tmux list-sessions -F ... | xargs kill-session lines) read helpers' output rather than re-duplicating the scripts so future naming rules change in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/bmad-story-automator/data/crash-recovery.md`:
- Around line 24-28: Add the missing diagnostic key
session_state.unexpected_schema_version to the malformed-state list described
for monitor-session --json so it is treated the same as
session_state.invalid_json, session_state.invalid_type, and
session_state.unreadable (i.e., classify it as a runtime-state diagnostic and
include it in the guidance to verify workflow truth from story files and
sprint-status.yaml before retrying).
In
`@skills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.py`:
- Around line 122-126: The catch-all except maps every build_agents_file failure
to an "invalid_agent_config" pointing at "config-json", which misattributes
failures from state-file, complexity-file, or output; change the except handler
to inspect the exception or context returned by build_agents_file (or re-raise a
custom exception carrying the failing input) and call agent_plan_error with the
correct field (e.g., "state-file", "complexity-file", or "output") and include
the actual failing input path in issues_from_exception so diagnostics preserve
the real failing file; update the logic around build_agents_file,
agent_plan_error, and issues_from_exception to propagate and log the true source
of the error rather than always using "config-json".
- Around line 316-317: The default for "defaultFallback" is currently the string
"false", which can be serialized as a literal value and break resolve_agent();
change the default to a boolean False by using config.get("defaultFallback",
False) and, if needed, normalize incoming values (e.g., convert "false"/"true"
strings to booleans) so resolve_agent() always receives a real boolean or a
proper agent identifier rather than the string "false".
In
`@skills/bmad-story-automator/src/story_automator/commands/orchestrator_parse.py`:
- Around line 41-46: The current except block lumps
FileNotFoundError/json.JSONDecodeError/ValueError/PolicyError into a
parse-contract error with field "parse.schemaPath"; separate those so parse
failures still call print_json(parse_failure_payload("parse_contract_invalid",
issues_from_exception(..., source="parse-contract", field="parse.schemaPath")))
for parsing-related exceptions (FileNotFoundError, json.JSONDecodeError,
ValueError) but handle PolicyError and state/runtime errors coming from
load_runtime_policy and step_contract with their own payloads (e.g.,
source="runtime-policy" or "step-contract" and an appropriate field like
"runtime.policy" or "state.file"), i.e., add distinct except clauses for
PolicyError and any state-related FileNotFoundError raised by
load_runtime_policy/step_contract and call
print_json(parse_failure_payload(...)) with the correct source/field for those
exceptions instead of reusing "parse.schemaPath".
In `@skills/bmad-story-automator/src/story_automator/core/agent_plan.py`:
- Around line 30-33: The code currently uses "complexity =
story.get('complexity') or {}" which coerces falsy but present values (e.g., "",
0, False) into missing; change to "complexity = story.get('complexity')" and
adjust the subsequent check to only validate when complexity is not None (e.g.,
"if complexity is not None and not isinstance(complexity, dict):
issues.append(_issue(...))"), so explicit invalid falsy values trigger the type
diagnostic via the existing _issue call instead of being treated as absent.
In `@skills/bmad-story-automator/src/story_automator/core/diagnostics.py`:
- Around line 80-87: The DiagnosticIssue created in issues_from_exception stores
raw str(exc) into message which bypasses redaction; change issues_from_exception
so that the same redaction/transformation used for DiagnosticIssue.actual is
applied to the message (e.g., call the shared redactor/serializer or helper used
elsewhere when building DiagnosticIssue.actual) so both actual and message
contain redacted output; update the DiagnosticIssue construction in
issues_from_exception to pass the redacted version for message (and ensure
message falls back to the exception class name if redaction yields empty).
In `@skills/bmad-story-automator/src/story_automator/core/session_state.py`:
- Around line 25-28: The except blocks that currently catch (OSError,
json.JSONDecodeError) around the read_text(target) + json.loads calls must also
catch UnicodeDecodeError so text-decoding failures fall back to {} instead of
bubbling up; update the exception tuples (the try that assigns raw =
json.loads(read_text(target)) and the similar try/except later) to include
UnicodeDecodeError (or add a separate except UnicodeDecodeError) so read_text,
json.loads and the target handling in session_state.py handle decoding errors
the same as unreadable files.
In `@skills/bmad-story-automator/src/story_automator/core/state_validation.py`:
- Around line 115-119: The function has_runtime_command_config treats
whitespace-only strings as valid; update it so that string values are accepted
only if ai_command is a non-empty string after stripping whitespace, and for
list values accept only if the list contains at least one non-empty (after
strip) string element; otherwise fall back to calling
_has_agent_config_block(frontmatter). Modify has_runtime_command_config to
perform isinstance checks for str and list and use .strip() on string elements
to reject whitespace-only entries.
In `@skills/bmad-story-automator/steps-c/step-02b-preflight-finalize.md`:
- Around line 77-82: The two separate date -u calls can yield inconsistent
timestamps; capture a single timestamp into a variable (e.g., TS_NOW) and reuse
it for the orchestrator-helper state-update --set lastUpdated and the echo
append to "{outputFile}" so both entries use the exact same ISO UTC time; update
the commands around the state-update invocation (state-update) and the echo line
that appends to "{outputFile}" to reference that variable instead of calling
date twice.
---
Nitpick comments:
In `@skills/bmad-story-automator/data/tmux-commands.md`:
- Around line 39-70: Replace the repeated inline derivation blocks that set
project_slug, project_hash, and session_name with calls to small helper commands
or shell functions (e.g., a get_project_slug and get_project_hash helper or
single sa-project-id command) and update every snippet that currently contains
the Python/hash and basename pipelines (the blocks that assign project_slug and
project_hash and the session_name usage) to invoke those helpers instead; ensure
the tmux examples (the tmux list-sessions grep and tmux list-sessions -F ... |
xargs kill-session lines) read helpers' output rather than re-duplicating the
scripts so future naming rules change in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aac36e51-5224-48f0-bef9-b237f2f801ff
📒 Files selected for processing (46)
docs/agents-and-monitoring.mddocs/cli-reference.mddocs/how-it-works.mddocs/plans/observability-validation/00-baseline-and-plan-reconciliation.mddocs/plans/observability-validation/01-diagnostics-contract.mddocs/plans/observability-validation/02-state-validation-and-transitions.mddocs/plans/observability-validation/03-parser-and-contract-boundaries.mddocs/plans/observability-validation/04-agent-complexity-and-story-boundaries.mddocs/plans/observability-validation/05-session-runtime-diagnostics.mddocs/plans/observability-validation/06-e2e-docs-and-release-readiness.mddocs/plans/observability-validation/README.mddocs/plans/observability-validation/TODO.mddocs/plans/observability-validation/handoff-log-archive-phase-00-04.mddocs/plans/observability-validation/handoff-log.mddocs/plans/observability-validation/implementation-notes.mddocs/state-and-resume.mddocs/troubleshooting.mdscripts/smoke-test.shskills/bmad-story-automator/data/crash-recovery.mdskills/bmad-story-automator/data/monitoring-pattern.mdskills/bmad-story-automator/data/tmux-commands.mdskills/bmad-story-automator/src/story_automator/cli.pyskills/bmad-story-automator/src/story_automator/commands/agent_config_cmd.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_parse.pyskills/bmad-story-automator/src/story_automator/commands/state.pyskills/bmad-story-automator/src/story_automator/commands/tmux.pyskills/bmad-story-automator/src/story_automator/core/agent_plan.pyskills/bmad-story-automator/src/story_automator/core/diagnostics.pyskills/bmad-story-automator/src/story_automator/core/parse_contracts.pyskills/bmad-story-automator/src/story_automator/core/review_verify.pyskills/bmad-story-automator/src/story_automator/core/session_state.pyskills/bmad-story-automator/src/story_automator/core/state_validation.pyskills/bmad-story-automator/src/story_automator/core/tmux_runtime.pyskills/bmad-story-automator/steps-c/step-02b-preflight-finalize.mdskills/bmad-story-automator/steps-v/step-v-01-check.mdtests/__init__.pytests/test_agent_plan.pytests/test_cli_contracts.pytests/test_diagnostics.pytests/test_diagnostics_e2e.pytests/test_orchestrator_parse.pytests/test_state_validation.pytests/test_success_verifiers.pytests/test_tmux_runtime.py
Collaborator
Author
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
skills/bmad-story-automator/src/story_automator/commands/orchestrator_parse.py (1)
38-59: 💤 Low valueMessage-based error routing may be fragile.
The routing at lines 40-41 depends on specific substrings in exception messages (
"parse schema"or"policy data file missing"). If the exception message wording changes upstream, the error will be misclassified asruntime_policy_invalidinstead ofparse_contract_invalid.Consider using distinct exception types or error codes rather than message inspection if the upstream module can be modified. However, if this is a deliberate pragmatic choice and the upstream messages are stable, this is acceptable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/bmad-story-automator/src/story_automator/commands/orchestrator_parse.py` around lines 38 - 59, The current PolicyError handling in orchestrator_parse.py relies on fragile substring checks of str(exc) to decide between parse_contract_invalid and runtime_policy_invalid; replace this brittle logic by using a concrete discriminator on the exception (e.g. an explicit exception subclass or a stable error code/attribute) instead: either have the upstream raise/attach a ParseSchemaError or an error.code like "parse_schema"/"policy_data_missing", or add a local classify_policy_error(exc) helper that inspects exc.code or isinstance(exc, ParseSchemaError) and returns the correct category; then route to print_json(parse_failure_payload(...)) using that discriminator (keeping the same issues_from_exception(...) usage) so decision logic no longer depends on message text.skills/bmad-story-automator/src/story_automator/core/agent_plan.py (1)
229-230: 💤 Low valueUnused call to
normalize_fallback_value.The call on line 230 has no side effect and its return value is discarded.
normalize_fallback_valueperforms string normalization but doesn't validate or raise exceptions, so this call serves no purpose.Suggested fix
if not (fallback is False or isinstance(fallback, str)): issues.append(_issue("invalid_type", f"{task_field}.fallback", "false or string", fallback, f"{task} fallback must be false or a string")) - elif isinstance(fallback, str): - normalize_fallback_value(fallback)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/bmad-story-automator/src/story_automator/core/agent_plan.py` around lines 229 - 230, The call to normalize_fallback_value(fallback) is unused because its return value is discarded; either remove the call or use its result. Update the branch handling (the elif that checks isinstance(fallback, str)) to assign the normalized string back (e.g., fallback = normalize_fallback_value(fallback)) or otherwise pass the normalized value into the downstream code that consumes fallback so the normalization has an effect; ensure you reference normalize_fallback_value and the local variable fallback in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@skills/bmad-story-automator/src/story_automator/commands/orchestrator_parse.py`:
- Around line 38-59: The current PolicyError handling in orchestrator_parse.py
relies on fragile substring checks of str(exc) to decide between
parse_contract_invalid and runtime_policy_invalid; replace this brittle logic by
using a concrete discriminator on the exception (e.g. an explicit exception
subclass or a stable error code/attribute) instead: either have the upstream
raise/attach a ParseSchemaError or an error.code like
"parse_schema"/"policy_data_missing", or add a local classify_policy_error(exc)
helper that inspects exc.code or isinstance(exc, ParseSchemaError) and returns
the correct category; then route to print_json(parse_failure_payload(...)) using
that discriminator (keeping the same issues_from_exception(...) usage) so
decision logic no longer depends on message text.
In `@skills/bmad-story-automator/src/story_automator/core/agent_plan.py`:
- Around line 229-230: The call to normalize_fallback_value(fallback) is unused
because its return value is discarded; either remove the call or use its result.
Update the branch handling (the elif that checks isinstance(fallback, str)) to
assign the normalized string back (e.g., fallback =
normalize_fallback_value(fallback)) or otherwise pass the normalized value into
the downstream code that consumes fallback so the normalization has an effect;
ensure you reference normalize_fallback_value and the local variable fallback in
your change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da7decd1-ae17-479a-8468-dc05dcfb3ad8
📒 Files selected for processing (20)
skills/bmad-story-automator/data/crash-recovery.mdskills/bmad-story-automator/data/tmux-commands.mdskills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_parse.pyskills/bmad-story-automator/src/story_automator/commands/state.pyskills/bmad-story-automator/src/story_automator/commands/tmux.pyskills/bmad-story-automator/src/story_automator/core/agent_config.pyskills/bmad-story-automator/src/story_automator/core/agent_config_frontmatter.pyskills/bmad-story-automator/src/story_automator/core/agent_plan.pyskills/bmad-story-automator/src/story_automator/core/diagnostics.pyskills/bmad-story-automator/src/story_automator/core/monitoring.pyskills/bmad-story-automator/src/story_automator/core/session_state.pyskills/bmad-story-automator/src/story_automator/core/state_validation.pyskills/bmad-story-automator/steps-c/step-02b-preflight-finalize.mdtests/test_agent_plan.pytests/test_diagnostics.pytests/test_orchestrator_parse.pytests/test_retro_agent.pytests/test_state_validation.pytests/test_tmux_runtime.py
✅ Files skipped from review due to trivial changes (1)
- skills/bmad-story-automator/data/crash-recovery.md
Collaborator
Author
|
@augmentcode review please |
🤖 Augment PR SummarySummary: This PR adds structured, field-addressable diagnostics across the story automator's parser, state, agent-plan, and session/tmux boundaries. It introduces a shared Changes:
Technical Notes: All changes are additive — legacy output fields are preserved. Parse success payloads remain unchanged. CSV status commands are untouched. The opt-in event channel avoids breaking scripts that parse stdout. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
Collaborator
Author
|
@augmentcode review please |
|
✅ Actions performedFull review triggered. |
|
This pull request is abnormally large and would use a significant amount of tokens to review. If you still wish to review it, comment "augment review" and we will review it. |
Collaborator
Author
|
augment review |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/bmad-story-automator/src/story_automator/core/diagnostics.py (1)
65-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact event-level
messagebefore serialization.Line 69 writes
event.messageunredacted. If any caller passes dynamic error text, this can leak sensitive/path data to diagnostics logs.🔧 Proposed fix
def serialize_event(event: DiagnosticEvent) -> dict[str, Any]: return { "name": event.name, "source": event.source, - "message": event.message, + "message": redact_actual(event.message), "severity": event.severity, "issues": serialize_issues(event.issues), "context": redact_actual(event.context), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/bmad-story-automator/src/story_automator/core/diagnostics.py` around lines 65 - 73, serialize_event currently includes event.message unredacted which can leak sensitive data; update serialize_event (in the serialize_event function) to pass the message through the existing redact_actual function (e.g., use redact_actual(event.message)) before putting it in the returned dict, ensuring the "message" field is redacted like "context" and keeping other fields (name, source, severity, issues) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_diagnostics.py`:
- Around line 4-5: Add an explicit import for the unittest.mock module so the
test file is self-contained; specifically, at the top of
tests/test_diagnostics.py add "import unittest.mock" (or otherwise ensure
unittest.mock is imported) because later code calls
unittest.mock.patch.dict(...) — update the imports block that currently contains
"import tempfile" and "import unittest" to include the explicit unittest.mock
import.
---
Outside diff comments:
In `@skills/bmad-story-automator/src/story_automator/core/diagnostics.py`:
- Around line 65-73: serialize_event currently includes event.message unredacted
which can leak sensitive data; update serialize_event (in the serialize_event
function) to pass the message through the existing redact_actual function (e.g.,
use redact_actual(event.message)) before putting it in the returned dict,
ensuring the "message" field is redacted like "context" and keeping other fields
(name, source, severity, issues) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65372596-1476-4c8f-9f61-ae663e010077
📒 Files selected for processing (31)
docs/cli-reference.mddocs/plans/observability-validation/07-review-remediation.mddocs/plans/observability-validation/08-diagnostic-redaction-completion.mddocs/plans/observability-validation/README.mddocs/plans/observability-validation/TODO.mddocs/plans/observability-validation/TODO/phase-08.mddocs/plans/observability-validation/gate-map.mddocs/plans/observability-validation/handoff-log.mddocs/plans/observability-validation/implementation-notes.mdskills/bmad-story-automator/src/story_automator/cli.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_parse.pyskills/bmad-story-automator/src/story_automator/commands/state.pyskills/bmad-story-automator/src/story_automator/commands/tmux.pyskills/bmad-story-automator/src/story_automator/commands/validate_story_creation.pyskills/bmad-story-automator/src/story_automator/core/agent_plan.pyskills/bmad-story-automator/src/story_automator/core/diagnostics.pyskills/bmad-story-automator/src/story_automator/core/monitoring.pyskills/bmad-story-automator/src/story_automator/core/orchestration_events.pyskills/bmad-story-automator/src/story_automator/core/parse_contracts.pyskills/bmad-story-automator/src/story_automator/core/runtime_policy.pyskills/bmad-story-automator/src/story_automator/core/state_validation.pytests/test_agent_plan.pytests/test_cli_contracts.pytests/test_diagnostics.pytests/test_diagnostics_e2e.pytests/test_orchestrator_parse.pytests/test_state_policy_metadata.pytests/test_state_validation.pytests/test_success_verifiers.py
✅ Files skipped from review due to trivial changes (6)
- docs/plans/observability-validation/07-review-remediation.md
- docs/plans/observability-validation/08-diagnostic-redaction-completion.md
- docs/plans/observability-validation/gate-map.md
- docs/plans/observability-validation/README.md
- docs/plans/observability-validation/TODO.md
- docs/plans/observability-validation/implementation-notes.md
Collaborator
Author
|
augment review |
Collaborator
Author
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@skills/bmad-story-automator/src/story_automator/commands/agent_config_cmd.py`:
- Around line 96-104: The wrapper _load_presets_or_report currently catches
json.JSONDecodeError and OSError but misses UnicodeDecodeError (a separate
exception) so UTF-8 decode failures aren’t returned as the structured
presets_file_error; update _load_presets_or_report to catch UnicodeDecodeError
(e.g., add an except UnicodeDecodeError as exc:) and call print_json({"ok":
False, "error": "presets_file_error", "reason": str(exc)}) and return None (or
alternatively combine it with the OSError handling) so decode errors from
load_presets_file are reported consistently.
In `@skills/bmad-story-automator/src/story_automator/commands/orchestrator.py`:
- Around line 305-308: The current re.subn call (using
re.subn(rf"(?m)^{re.escape(key)}:.*$", ... ) on variable text) updates the
entire markdown, causing body lines to be overwritten; restrict replacements to
the YAML frontmatter only by extracting the frontmatter block (delimited by
leading and trailing '---' or '---\\n'), run the re.subn and updated.append(key)
against that frontmatter string (using the existing key/value logic and
re.subn), then reassemble text = frontmatter + rest_of_document; update
references to text, key, value, updated and the re.subn call in orchestrator.py
so only the frontmatter is modified.
In
`@skills/bmad-story-automator/src/story_automator/core/agent_config_frontmatter.py`:
- Around line 10-12: The code currently uses stripped = raw_line.strip() then
stripped.startswith("agentConfig:"), which treats indented keys like "
agentConfig:" as top-level; change the detection to be left-whitespace sensitive
by checking raw_line.startswith("agentConfig:") (i.e., do not remove leading
whitespace) before calling _extract_agent_config_block(frontmatter.splitlines(),
index), so only truly top-level unindented "agentConfig:" lines trigger
extraction; update the raw_line handling accordingly while keeping the call to
_extract_agent_config_block with frontmatter and index.
- Around line 38-45: The loop over enumerate(lines) in
agent_config_frontmatter.py allows lines like " \tkey: value" because it only
checks for tabs at line start earlier; update the loop that calls
_strip_inline_yaml_comment and computes indent to also reject any tabs in the
indentation portion (or anywhere in the line after stripping inline comments).
Concretely, after computing indent = len(line) - len(line.lstrip(" ")), assert
that line[:indent] contains only spaces (no '\t') and/or if '\t' in line: raise
the same ValueError (or a clearer one) so lines with tabs after spaces are
rejected consistently by this function.
In `@skills/bmad-story-automator/src/story_automator/core/diagnostics.py`:
- Around line 21-25: The current regexes ABSOLUTE_PATH_WITH_EXT_RE and
ABSOLUTE_PATH_RE only match Unix-style paths and must be extended to also detect
Windows absolute paths (e.g., "C:\Users\..." or "C:/Users/...") to avoid leaking
local Windows paths; update the definitions for ABSOLUTE_PATH_WITH_EXT_RE and
ABSOLUTE_PATH_RE to include alternative branches that match a drive letter plus
colon and backslash/forward slash patterns and ensure backslashes are correctly
escaped in the raw string, then apply the same updated regexes where those
symbols are redefined/used later (the other occurrence referenced in the
comment) so both the with-extension and generic absolute-path redaction cover
Windows paths.
In `@skills/bmad-story-automator/src/story_automator/core/state_validation.py`:
- Around line 145-148: The parsed "--set" handler currently returns the raw key
which allows whitespace-padded keys to bypass validation; update the block
around "key, value = raw.split('=', 1)" to normalize the key by calling key =
key.strip() (and optionally strip value as well) before returning so the
function (and downstream validation in state transition checks) receives a
trimmed key; keep the existing error branch that calls
state_update_argument_error_payload(raw).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9bbb554-a626-414a-8257-d556cc99fa2e
📒 Files selected for processing (58)
docs/agents-and-monitoring.mddocs/cli-reference.mddocs/how-it-works.mddocs/plans/observability-validation/00-baseline-and-plan-reconciliation.mddocs/plans/observability-validation/01-diagnostics-contract.mddocs/plans/observability-validation/02-state-validation-and-transitions.mddocs/plans/observability-validation/03-parser-and-contract-boundaries.mddocs/plans/observability-validation/04-agent-complexity-and-story-boundaries.mddocs/plans/observability-validation/05-session-runtime-diagnostics.mddocs/plans/observability-validation/06-e2e-docs-and-release-readiness.mddocs/plans/observability-validation/07-review-remediation.mddocs/plans/observability-validation/08-diagnostic-redaction-completion.mddocs/plans/observability-validation/README.mddocs/plans/observability-validation/TODO.mddocs/plans/observability-validation/TODO/phase-08.mddocs/plans/observability-validation/gate-map.mddocs/plans/observability-validation/handoff-log-archive-phase-00-04.mddocs/plans/observability-validation/handoff-log.mddocs/plans/observability-validation/implementation-notes.mddocs/state-and-resume.mddocs/troubleshooting.mdscripts/smoke-test.shskills/bmad-story-automator/data/crash-recovery.mdskills/bmad-story-automator/data/monitoring-pattern.mdskills/bmad-story-automator/data/tmux-commands.mdskills/bmad-story-automator/src/story_automator/cli.pyskills/bmad-story-automator/src/story_automator/commands/agent_config_cmd.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_parse.pyskills/bmad-story-automator/src/story_automator/commands/state.pyskills/bmad-story-automator/src/story_automator/commands/tmux.pyskills/bmad-story-automator/src/story_automator/commands/validate_story_creation.pyskills/bmad-story-automator/src/story_automator/core/agent_config.pyskills/bmad-story-automator/src/story_automator/core/agent_config_frontmatter.pyskills/bmad-story-automator/src/story_automator/core/agent_plan.pyskills/bmad-story-automator/src/story_automator/core/diagnostics.pyskills/bmad-story-automator/src/story_automator/core/monitoring.pyskills/bmad-story-automator/src/story_automator/core/orchestration_events.pyskills/bmad-story-automator/src/story_automator/core/parse_contracts.pyskills/bmad-story-automator/src/story_automator/core/review_verify.pyskills/bmad-story-automator/src/story_automator/core/runtime_policy.pyskills/bmad-story-automator/src/story_automator/core/session_state.pyskills/bmad-story-automator/src/story_automator/core/state_validation.pyskills/bmad-story-automator/src/story_automator/core/tmux_runtime.pyskills/bmad-story-automator/steps-c/step-02b-preflight-finalize.mdskills/bmad-story-automator/steps-v/step-v-01-check.mdtests/__init__.pytests/test_agent_plan.pytests/test_cli_contracts.pytests/test_diagnostics.pytests/test_diagnostics_e2e.pytests/test_orchestrator_parse.pytests/test_retro_agent.pytests/test_state_policy_metadata.pytests/test_state_validation.pytests/test_success_verifiers.pytests/test_tmux_runtime.py
✅ Files skipped from review due to trivial changes (18)
- docs/plans/observability-validation/gate-map.md
- docs/state-and-resume.md
- skills/bmad-story-automator/data/monitoring-pattern.md
- docs/plans/observability-validation/03-parser-and-contract-boundaries.md
- docs/agents-and-monitoring.md
- docs/plans/observability-validation/08-diagnostic-redaction-completion.md
- docs/plans/observability-validation/00-baseline-and-plan-reconciliation.md
- docs/plans/observability-validation/07-review-remediation.md
- docs/cli-reference.md
- docs/plans/observability-validation/README.md
- docs/troubleshooting.md
- tests/init.py
- docs/how-it-works.md
- docs/plans/observability-validation/06-e2e-docs-and-release-readiness.md
- skills/bmad-story-automator/data/crash-recovery.md
- docs/plans/observability-validation/04-agent-complexity-and-story-boundaries.md
- docs/plans/observability-validation/02-state-validation-and-transitions.md
- docs/plans/observability-validation/TODO.md
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@augmentcode review |
Collaborator
Author
|
@augmentcode review |
|
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@augmentcode review |
|
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@augmentcode review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/bmad-story-automator/src/story_automator/commands/tmux_monitor.py`:
- Around line 11-18: The error text for numeric options is misleading because
parse_monitor_int_option (and other callers) use a variable minimum (sometimes
0) but _invalid_numeric_option always emits "positive integer"; update the
behavior so the message matches the minimum: change _invalid_numeric_option to
accept an optional minimum parameter (e.g., minimum: int = 1) and construct the
error string as "positive integer" when minimum == 1, "non-negative integer"
when minimum == 0, or "integer >= {minimum}" otherwise; then update
parse_monitor_int_option to pass its minimum through when calling
_invalid_numeric_option so flags like --initial-wait show the correct guidance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a45abec-3075-4437-9f39-b8cee967957d
📒 Files selected for processing (62)
docs/agents-and-monitoring.mddocs/cli-reference.mddocs/how-it-works.mddocs/plans/observability-validation/00-baseline-and-plan-reconciliation.mddocs/plans/observability-validation/01-diagnostics-contract.mddocs/plans/observability-validation/02-state-validation-and-transitions.mddocs/plans/observability-validation/03-parser-and-contract-boundaries.mddocs/plans/observability-validation/04-agent-complexity-and-story-boundaries.mddocs/plans/observability-validation/05-session-runtime-diagnostics.mddocs/plans/observability-validation/06-e2e-docs-and-release-readiness.mddocs/plans/observability-validation/07-review-remediation.mddocs/plans/observability-validation/08-diagnostic-redaction-completion.mddocs/plans/observability-validation/README.mddocs/plans/observability-validation/TODO.mddocs/plans/observability-validation/TODO/phase-08.mddocs/plans/observability-validation/gate-map.mddocs/plans/observability-validation/handoff-log-archive-phase-00-04.mddocs/plans/observability-validation/handoff-log.mddocs/plans/observability-validation/implementation-notes.mddocs/state-and-resume.mddocs/troubleshooting.mdscripts/smoke-test.shskills/bmad-story-automator/data/crash-recovery.mdskills/bmad-story-automator/data/monitoring-pattern.mdskills/bmad-story-automator/data/tmux-commands.mdskills/bmad-story-automator/src/story_automator/cli.pyskills/bmad-story-automator/src/story_automator/commands/agent_config_cmd.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_parse.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_state.pyskills/bmad-story-automator/src/story_automator/commands/state.pyskills/bmad-story-automator/src/story_automator/commands/tmux.pyskills/bmad-story-automator/src/story_automator/commands/tmux_monitor.pyskills/bmad-story-automator/src/story_automator/commands/validate_story_creation.pyskills/bmad-story-automator/src/story_automator/core/agent_config.pyskills/bmad-story-automator/src/story_automator/core/agent_config_frontmatter.pyskills/bmad-story-automator/src/story_automator/core/agent_plan.pyskills/bmad-story-automator/src/story_automator/core/diagnostics.pyskills/bmad-story-automator/src/story_automator/core/monitoring.pyskills/bmad-story-automator/src/story_automator/core/orchestration_events.pyskills/bmad-story-automator/src/story_automator/core/parse_contracts.pyskills/bmad-story-automator/src/story_automator/core/review_verify.pyskills/bmad-story-automator/src/story_automator/core/runtime_policy.pyskills/bmad-story-automator/src/story_automator/core/session_state.pyskills/bmad-story-automator/src/story_automator/core/state_validation.pyskills/bmad-story-automator/src/story_automator/core/tmux_runtime.pyskills/bmad-story-automator/steps-c/step-02b-preflight-finalize.mdskills/bmad-story-automator/steps-v/step-v-01-check.mdtests/__init__.pytests/test_agent_config_model.pytests/test_agent_plan.pytests/test_cli_contracts.pytests/test_diagnostics.pytests/test_diagnostics_e2e.pytests/test_orchestrator_parse.pytests/test_retro_agent.pytests/test_state_policy_metadata.pytests/test_state_validation.pytests/test_stop_hooks.pytests/test_success_verifiers.pytests/test_tmux_runtime.py
✅ Files skipped from review due to trivial changes (17)
- docs/how-it-works.md
- docs/plans/observability-validation/00-baseline-and-plan-reconciliation.md
- docs/plans/observability-validation/TODO.md
- docs/plans/observability-validation/05-session-runtime-diagnostics.md
- docs/plans/observability-validation/TODO/phase-08.md
- tests/init.py
- skills/bmad-story-automator/data/monitoring-pattern.md
- docs/state-and-resume.md
- docs/plans/observability-validation/gate-map.md
- docs/agents-and-monitoring.md
- docs/troubleshooting.md
- docs/plans/observability-validation/README.md
- docs/plans/observability-validation/04-agent-complexity-and-story-boundaries.md
- docs/plans/observability-validation/06-e2e-docs-and-release-readiness.md
- docs/plans/observability-validation/08-diagnostic-redaction-completion.md
- docs/plans/observability-validation/02-state-validation-and-transitions.md
- skills/bmad-story-automator/data/crash-recovery.md
🚧 Files skipped from review as they are similar to previous changes (33)
- docs/plans/observability-validation/07-review-remediation.md
- skills/bmad-story-automator/src/story_automator/core/review_verify.py
- docs/plans/observability-validation/03-parser-and-contract-boundaries.md
- skills/bmad-story-automator/steps-c/step-02b-preflight-finalize.md
- tests/test_stop_hooks.py
- skills/bmad-story-automator/steps-v/step-v-01-check.md
- tests/test_agent_config_model.py
- tests/test_tmux_runtime.py
- skills/bmad-story-automator/src/story_automator/commands/state.py
- tests/test_state_policy_metadata.py
- skills/bmad-story-automator/src/story_automator/core/runtime_policy.py
- skills/bmad-story-automator/src/story_automator/core/session_state.py
- skills/bmad-story-automator/src/story_automator/core/orchestration_events.py
- skills/bmad-story-automator/data/tmux-commands.md
- skills/bmad-story-automator/src/story_automator/commands/validate_story_creation.py
- skills/bmad-story-automator/src/story_automator/core/monitoring.py
- skills/bmad-story-automator/src/story_automator/core/tmux_runtime.py
- tests/test_diagnostics_e2e.py
- docs/cli-reference.md
- skills/bmad-story-automator/src/story_automator/core/state_validation.py
- skills/bmad-story-automator/src/story_automator/cli.py
- skills/bmad-story-automator/src/story_automator/commands/agent_config_cmd.py
- skills/bmad-story-automator/src/story_automator/core/agent_config.py
- skills/bmad-story-automator/src/story_automator/core/agent_config_frontmatter.py
- tests/test_retro_agent.py
- skills/bmad-story-automator/src/story_automator/core/parse_contracts.py
- tests/test_orchestrator_parse.py
- skills/bmad-story-automator/src/story_automator/commands/orchestrator_state.py
- skills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.py
- skills/bmad-story-automator/src/story_automator/commands/orchestrator_parse.py
- skills/bmad-story-automator/src/story_automator/core/agent_plan.py
- skills/bmad-story-automator/src/story_automator/commands/orchestrator.py
- skills/bmad-story-automator/src/story_automator/commands/tmux.py
Collaborator
Author
|
@augmentcode review |
|
✅ Action performedFull review finished. |
Collaborator
Author
|
@augmentcode review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
skills/bmad-story-automator/data/tmux-commands.md (1)
124-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the placeholder in the full-output example.
<installed-skill-root>is a literal token, so this snippet no longer resolves the installed helper. Keep the{installed-skill-root}placeholder here to match the rest of the doc and make the copy-paste command work.Suggested fix
-script="$(printf "%s" "{project_root}/<installed-skill-root>/bmad-story-automator/scripts/story-automator")" +script="$(printf "%s" "{project_root}/{installed-skill-root}/bmad-story-automator/scripts/story-automator")"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/bmad-story-automator/data/tmux-commands.md` around lines 124 - 126, The example's placeholder is wrong: change the literal token "<installed-skill-root>" to the brace-style placeholder "{installed-skill-root}" in the script assignment so the `script` variable (assigned via printf) matches the rest of the doc and the copy-paste command (`"$script" tmux-status-check "SESSION_NAME" --full`) resolves the installed helper.skills/bmad-story-automator/src/story_automator/cli.py (1)
134-142:⚠️ Potential issue | 🟠 MajorCatch
UnicodeDecodeErrorin thefile_read_failedpath.
_cmd_parse_storyhandlesOSErrorby returning{"error":"file_read_failed", ...}, butUnicodeDecodeErroris not anOSError(issubclass(UnicodeDecodeError, OSError) == False) and instead subclassesValueError, so it will fall intoexcept ValueError as excand return rawerror: str(exc)at lines 134-142.Suggested fix
- except OSError as exc: + except (OSError, UnicodeDecodeError) as exc: print_json({"ok": False, "error": "file_read_failed", "reason": str(redact_actual(str(exc)))}) return 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/bmad-story-automator/src/story_automator/cli.py` around lines 134 - 142, _unicode decode errors are currently handled by the ValueError except and return a raw error string; modify _cmd_parse_story's exception handling so UnicodeDecodeError is caught and treated like file read failures by adding an explicit "except UnicodeDecodeError as exc:" block (placed before the generic ValueError except) that calls print_json({"ok": False, "error": "file_read_failed", "reason": str(redact_actual(str(exc)))}) and returns 1, mirroring the OSError path and ensuring print_json and redact_actual are used consistently.
♻️ Duplicate comments (1)
docs/plans/observability-validation/handoff-log-archive-phase-00-04.md (1)
341-341:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove workstation-specific absolute path from committed docs.
Line 341 still contains a local absolute path (
/Users/joon/...), which leaks contributor workstation details and reduces portability. Replace it with a sanitized repo-relative placeholder.Suggested fix
-- Use current repository `/Users/joon/.codex/worktrees/9b27/bmad-story-automator`. +- Use current repository `<REPO_ROOT>/bmad-story-automator`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/observability-validation/handoff-log-archive-phase-00-04.md` at line 341, Remove the workstation-specific absolute path "/Users/joon/.codex/worktrees/9b27/bmad-story-automator" from docs/plans/observability-validation/handoff-log-archive-phase-00-04.md (line containing that string) and replace it with a sanitized repo-relative placeholder such as "./<repo-root>/bmad-story-automator" or "<repo-root>/worktree-name" (or simply the repository-relative path), then save and commit the change so no local machine paths are committed.
🧹 Nitpick comments (1)
skills/bmad-story-automator/src/story_automator/commands/orchestrator.py (1)
467-469: ⚖️ Poor tradeoffField inference by message prefix is fragile.
The code infers
fieldby checking if the exception message starts with"--state-file requires"or"--output-file requires". If the upstreamPolicyErrormessage format changes (Line 446, Line 457), this heuristic will silently fail andfieldwill default to empty string.A more robust approach would be to add a
fieldattribute toPolicyErrorat the raise site (lines 446, 457) or use distinct exception types for flag-parsing vs contract-resolution failures, eliminating the need for message parsing.However, this refactor would require broader changes to the exception handling pattern. For now, the heuristic is localized and testable, and the fallback (
field="") still produces valid diagnostics with slightly less precision.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/bmad-story-automator/src/story_automator/commands/orchestrator.py` around lines 467 - 469, The current heuristic inspects the exception message to infer the CLI flag name which is fragile; add a stable field on the exception and use it when available: update the PolicyError (or the specific exception class raised at the flag-parsing sites) to accept and set a 'field' attribute where the errors are raised (the two raise sites mentioned in the review), then change the handler in orchestrator.py that builds 'field' to prefer getattr(exc, "field", None) and fall back to the existing message-prefix checks only if that attribute is missing before calling verifier_exception_payload("verifier_contract_invalid", exc, ...). Ensure the new 'field' attribute uses the same canonical values ("--state-file", "--output-file") so diagnostics remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/bmad-story-automator/steps-v/step-v-01-check.md`:
- Around line 141-149: The current extraction in field_issues filters
structuredIssues down to only missing_field, invalid_value, and yaml_error,
dropping new types (e.g., enum/type/null-field); update the jq pipeline so it
does not use select(...) to whitelist types but instead iterates all
.structuredIssues[]? and formats each as "\(.type): \(.field // .message)"; keep
the existing fallback to .issues[]? when structuredIssues is empty so validation
still works.
In `@tests/test_tmux_runtime.py`:
- Around line 117-119: The test
test_tmux_command_module_stays_under_soft_size_limit currently enforces a soft
500-line limit as a hard CI gate; remove this assertion from
tests/test_tmux_runtime.py (or convert the test to a non-failing check such as
skipping it or marking it xfail) and instead document the 500-line soft target
or add it to non-blocking tooling (linter/pre-commit) so edits over 500 lines do
not fail unit tests; locate the function named
test_tmux_command_module_stays_under_soft_size_limit to apply the change.
---
Outside diff comments:
In `@skills/bmad-story-automator/data/tmux-commands.md`:
- Around line 124-126: The example's placeholder is wrong: change the literal
token "<installed-skill-root>" to the brace-style placeholder
"{installed-skill-root}" in the script assignment so the `script` variable
(assigned via printf) matches the rest of the doc and the copy-paste command
(`"$script" tmux-status-check "SESSION_NAME" --full`) resolves the installed
helper.
In `@skills/bmad-story-automator/src/story_automator/cli.py`:
- Around line 134-142: _unicode decode errors are currently handled by the
ValueError except and return a raw error string; modify _cmd_parse_story's
exception handling so UnicodeDecodeError is caught and treated like file read
failures by adding an explicit "except UnicodeDecodeError as exc:" block (placed
before the generic ValueError except) that calls print_json({"ok": False,
"error": "file_read_failed", "reason": str(redact_actual(str(exc)))}) and
returns 1, mirroring the OSError path and ensuring print_json and redact_actual
are used consistently.
---
Duplicate comments:
In `@docs/plans/observability-validation/handoff-log-archive-phase-00-04.md`:
- Line 341: Remove the workstation-specific absolute path
"/Users/joon/.codex/worktrees/9b27/bmad-story-automator" from
docs/plans/observability-validation/handoff-log-archive-phase-00-04.md (line
containing that string) and replace it with a sanitized repo-relative
placeholder such as "./<repo-root>/bmad-story-automator" or
"<repo-root>/worktree-name" (or simply the repository-relative path), then save
and commit the change so no local machine paths are committed.
---
Nitpick comments:
In `@skills/bmad-story-automator/src/story_automator/commands/orchestrator.py`:
- Around line 467-469: The current heuristic inspects the exception message to
infer the CLI flag name which is fragile; add a stable field on the exception
and use it when available: update the PolicyError (or the specific exception
class raised at the flag-parsing sites) to accept and set a 'field' attribute
where the errors are raised (the two raise sites mentioned in the review), then
change the handler in orchestrator.py that builds 'field' to prefer getattr(exc,
"field", None) and fall back to the existing message-prefix checks only if that
attribute is missing before calling
verifier_exception_payload("verifier_contract_invalid", exc, ...). Ensure the
new 'field' attribute uses the same canonical values ("--state-file",
"--output-file") so diagnostics remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef768c15-f0b8-4eca-b19b-3f192b95391e
📒 Files selected for processing (62)
docs/agents-and-monitoring.mddocs/cli-reference.mddocs/how-it-works.mddocs/plans/observability-validation/00-baseline-and-plan-reconciliation.mddocs/plans/observability-validation/01-diagnostics-contract.mddocs/plans/observability-validation/02-state-validation-and-transitions.mddocs/plans/observability-validation/03-parser-and-contract-boundaries.mddocs/plans/observability-validation/04-agent-complexity-and-story-boundaries.mddocs/plans/observability-validation/05-session-runtime-diagnostics.mddocs/plans/observability-validation/06-e2e-docs-and-release-readiness.mddocs/plans/observability-validation/07-review-remediation.mddocs/plans/observability-validation/08-diagnostic-redaction-completion.mddocs/plans/observability-validation/README.mddocs/plans/observability-validation/TODO.mddocs/plans/observability-validation/TODO/phase-08.mddocs/plans/observability-validation/gate-map.mddocs/plans/observability-validation/handoff-log-archive-phase-00-04.mddocs/plans/observability-validation/handoff-log.mddocs/plans/observability-validation/implementation-notes.mddocs/state-and-resume.mddocs/troubleshooting.mdscripts/smoke-test.shskills/bmad-story-automator/data/crash-recovery.mdskills/bmad-story-automator/data/monitoring-pattern.mdskills/bmad-story-automator/data/tmux-commands.mdskills/bmad-story-automator/src/story_automator/cli.pyskills/bmad-story-automator/src/story_automator/commands/agent_config_cmd.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_parse.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_state.pyskills/bmad-story-automator/src/story_automator/commands/state.pyskills/bmad-story-automator/src/story_automator/commands/tmux.pyskills/bmad-story-automator/src/story_automator/commands/tmux_monitor.pyskills/bmad-story-automator/src/story_automator/commands/validate_story_creation.pyskills/bmad-story-automator/src/story_automator/core/agent_config.pyskills/bmad-story-automator/src/story_automator/core/agent_config_frontmatter.pyskills/bmad-story-automator/src/story_automator/core/agent_plan.pyskills/bmad-story-automator/src/story_automator/core/diagnostics.pyskills/bmad-story-automator/src/story_automator/core/monitoring.pyskills/bmad-story-automator/src/story_automator/core/orchestration_events.pyskills/bmad-story-automator/src/story_automator/core/parse_contracts.pyskills/bmad-story-automator/src/story_automator/core/review_verify.pyskills/bmad-story-automator/src/story_automator/core/runtime_policy.pyskills/bmad-story-automator/src/story_automator/core/session_state.pyskills/bmad-story-automator/src/story_automator/core/state_validation.pyskills/bmad-story-automator/src/story_automator/core/tmux_runtime.pyskills/bmad-story-automator/steps-c/step-02b-preflight-finalize.mdskills/bmad-story-automator/steps-v/step-v-01-check.mdtests/__init__.pytests/test_agent_config_model.pytests/test_agent_plan.pytests/test_cli_contracts.pytests/test_diagnostics.pytests/test_diagnostics_e2e.pytests/test_orchestrator_parse.pytests/test_retro_agent.pytests/test_state_policy_metadata.pytests/test_state_validation.pytests/test_stop_hooks.pytests/test_success_verifiers.pytests/test_tmux_runtime.py
Collaborator
Author
|
@augmentcode review |
|
✅ Action performedReview finished.
|
Collaborator
Author
|
@augmentcode review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/bmad-story-automator/src/story_automator/commands/tmux.py (1)
64-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
kill-alldocuments--all-projects, but that flag is currently ignored.The action scope is still derived only from
--project-only, so--all-projectshas no behavioral effect despite being in help output.Suggested fix
if action == "kill-all": - sessions, _ = tmux_list_sessions("--project-only" in args[1:]) + scope_args = args[1:] + project_only = "--all-projects" not in scope_args + if "--project-only" in scope_args: + project_only = True + sessions, _ = tmux_list_sessions(project_only) for session in sessions: tmux_kill_session(session)Also applies to: 125-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/bmad-story-automator/src/story_automator/commands/tmux.py` around lines 64 - 66, The `kill-all` action documents a `--all-projects` flag in its help text but the implementation in the condition passed to tmux_list_sessions only checks for `--project-only`, causing the `--all-projects` flag to be ignored. Update the logic where tmux_list_sessions is called to check for the presence of both `--all-projects` and `--project-only` flags in args and pass the appropriate scope parameter to tmux_list_sessions. Apply the same fix to the other occurrence at line 125 where this issue also exists.
🧹 Nitpick comments (4)
skills/bmad-story-automator/src/story_automator/core/agent_config.py (1)
143-168: 💤 Low valueRedundant
defaultModelcheck in the loop.
defaultModelis checked explicitly at line 159, then appears again in the loop at line 161. The second check is unreachable fordefaultModelsince the function already returnedTrueat line 160.♻️ Suggested cleanup
if "defaultModel" in config: return True - for key in ("defaultPrimary", "primary", "defaultFallback", "fallback", "defaultModel"): + for key in ("defaultPrimary", "primary", "defaultFallback", "fallback"): value = config.get(key) if value not in ("", [], {}, None): return True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/bmad-story-automator/src/story_automator/core/agent_config.py` around lines 143 - 168, In the has_agent_config_runtime_source function, the key "defaultModel" is checked redundantly both in the explicit if statement at lines 159-160 and again in the for loop at line 161 that iterates over the tuple of keys including "defaultModel". Since the function returns True at line 160 if "defaultModel" is present and truthy, the subsequent check in the for loop for "defaultModel" is unreachable. Remove "defaultModel" from the tuple of keys in the for loop since the explicit check above already handles this key.docs/plans/observability-validation/implementation-notes.md (1)
264-264: 💤 Low valueMinor: Remove redundant phrasing in Phase 01 description.
Line 264 uses "follow after" which is slightly redundant. Consider:
- Parser, agent plan, state transition, and session diagnostics follow once the shared diagnostics contract exists.Or:
- Parser, agent plan, state transition, and session diagnostics come after the shared diagnostics contract exists.This is a minor readability polish with no impact on meaning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/observability-validation/implementation-notes.md` at line 264, In the Phase 01 description, the phrase "follow after" on line 264 is redundant and can be improved for better readability. Replace "follow after" with either "follow once" or "come after" to make the sentence more concise and clear. The sentence should read either "Parser, agent plan, state transition, and session diagnostics follow once the shared diagnostics contract exists" or "Parser, agent plan, state transition, and session diagnostics come after the shared diagnostics contract exists".docs/plans/observability-validation/handoff-log.md (1)
148-149: 💤 Low valueConsider rewording repetitive sentence beginnings in results and decisions sections.
Multiple successive lines begin with "Verified" or "Updated", creating repetitive phrasing:
Examples:
- Lines 148-149: "Verified P2 finding... - Verified P2 finding..."
- Lines 302-304: "Updated
docs/.... - Updateddocs/......"Combining or restructuring these would improve readability without affecting content accuracy:
- Verified P2 findings: invalid state-update redacts... and verifier error echoes...This is a minor style improvement.
Also applies to: 302-304, 361-364
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/observability-validation/handoff-log.md` around lines 148 - 149, The markdown document has repetitive sentence beginnings in the results and decisions sections that reduce readability. Identify all consecutive bullet points that begin with the same verb (such as "Verified" or "Updated") at lines 148-149, 302-304, and 361-364, and combine or restructure them to eliminate the repetition. For lines with multiple "Verified" statements, merge them into a single bullet point listing multiple findings separated by "and". For consecutive "Updated" statements, either combine them into one entry or vary the sentence structure and phrasing so each line starts with different wording while preserving all the original content and meaning.docs/plans/observability-validation/handoff-log-archive-phase-00-04.md (1)
224-224: 💤 Low valueConsider rewording repetitive sentence structures for clarity.
Multiple successive lines begin with "Updated
file" or "Verifiedthing", which creates monotonous reading. While not incorrect, these can be combined or restructured slightly for better flow:Example rewrite:
- Updated docs, implementation notes, and handoff log (instead of three separate lines).This is a minor readability improvement and does not block the document.
Also applies to: 302-304, 361-364
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/observability-validation/handoff-log-archive-phase-00-04.md` at line 224, The document contains repetitive sentence structures where multiple consecutive bullet points begin with similar phrases like "Updated `file`" or "Verified `thing`", which creates monotonous reading. Identify all occurrences of this pattern throughout the document (including the section around line 224 and the additional sections noted in the comment) and consolidate related items into single bullet points where appropriate. For example, instead of three separate lines each beginning with "Updated", combine them into a single line that lists all the updated items together, separated by commas or "and". This will improve readability and flow without changing the factual content.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/bmad-story-automator/src/story_automator/core/diagnostics.py`:
- Around line 16-27: The secret assignment regex patterns
(SECRET_QUOTED_ASSIGNMENT_RE, SECRET_ASSIGNMENT_RE,
SECRET_PATH_VALUE_ASSIGNMENT_RE, and SECRET_PATH_PLACEHOLDER_ASSIGNMENT_RE) fail
to match quoted keys in JSON-like strings such as "api_key":"secret". Modify
each regex pattern to make the surrounding quotes optional by adjusting the
negative lookbehind and adding optional quote patterns before the
SECRET_KEY_PATTERN, so the patterns can detect and redact secrets whether the
keys are quoted like "api_key":"value" or unquoted like api_key:value.
In `@skills/bmad-story-automator/src/story_automator/core/monitoring.py`:
- Around line 57-58: The return statement at lines 57-58 in the monitoring
module bypasses sanitization by returning a valid-shaped structured_issue dict
directly without applying redaction or normalization, which can leak sensitive
raw values in JSON output. Instead of returning structured_issue immediately
when the shape validation passes, apply the same sanitization/normalization
function that is used elsewhere in the code (likely exists in the same function
or module for handling structured issues) to redact sensitive fields like actual
and message before returning the normalized result.
---
Outside diff comments:
In `@skills/bmad-story-automator/src/story_automator/commands/tmux.py`:
- Around line 64-66: The `kill-all` action documents a `--all-projects` flag in
its help text but the implementation in the condition passed to
tmux_list_sessions only checks for `--project-only`, causing the
`--all-projects` flag to be ignored. Update the logic where tmux_list_sessions
is called to check for the presence of both `--all-projects` and
`--project-only` flags in args and pass the appropriate scope parameter to
tmux_list_sessions. Apply the same fix to the other occurrence at line 125 where
this issue also exists.
---
Nitpick comments:
In `@docs/plans/observability-validation/handoff-log-archive-phase-00-04.md`:
- Line 224: The document contains repetitive sentence structures where multiple
consecutive bullet points begin with similar phrases like "Updated `file`" or
"Verified `thing`", which creates monotonous reading. Identify all occurrences
of this pattern throughout the document (including the section around line 224
and the additional sections noted in the comment) and consolidate related items
into single bullet points where appropriate. For example, instead of three
separate lines each beginning with "Updated", combine them into a single line
that lists all the updated items together, separated by commas or "and". This
will improve readability and flow without changing the factual content.
In `@docs/plans/observability-validation/handoff-log.md`:
- Around line 148-149: The markdown document has repetitive sentence beginnings
in the results and decisions sections that reduce readability. Identify all
consecutive bullet points that begin with the same verb (such as "Verified" or
"Updated") at lines 148-149, 302-304, and 361-364, and combine or restructure
them to eliminate the repetition. For lines with multiple "Verified" statements,
merge them into a single bullet point listing multiple findings separated by
"and". For consecutive "Updated" statements, either combine them into one entry
or vary the sentence structure and phrasing so each line starts with different
wording while preserving all the original content and meaning.
In `@docs/plans/observability-validation/implementation-notes.md`:
- Line 264: In the Phase 01 description, the phrase "follow after" on line 264
is redundant and can be improved for better readability. Replace "follow after"
with either "follow once" or "come after" to make the sentence more concise and
clear. The sentence should read either "Parser, agent plan, state transition,
and session diagnostics follow once the shared diagnostics contract exists" or
"Parser, agent plan, state transition, and session diagnostics come after the
shared diagnostics contract exists".
In `@skills/bmad-story-automator/src/story_automator/core/agent_config.py`:
- Around line 143-168: In the has_agent_config_runtime_source function, the key
"defaultModel" is checked redundantly both in the explicit if statement at lines
159-160 and again in the for loop at line 161 that iterates over the tuple of
keys including "defaultModel". Since the function returns True at line 160 if
"defaultModel" is present and truthy, the subsequent check in the for loop for
"defaultModel" is unreachable. Remove "defaultModel" from the tuple of keys in
the for loop since the explicit check above already handles this key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9cba169-1b01-4c15-9ee1-0901eb4663d8
📒 Files selected for processing (63)
docs/agents-and-monitoring.mddocs/cli-reference.mddocs/how-it-works.mddocs/plans/observability-validation/00-baseline-and-plan-reconciliation.mddocs/plans/observability-validation/01-diagnostics-contract.mddocs/plans/observability-validation/02-state-validation-and-transitions.mddocs/plans/observability-validation/03-parser-and-contract-boundaries.mddocs/plans/observability-validation/04-agent-complexity-and-story-boundaries.mddocs/plans/observability-validation/05-session-runtime-diagnostics.mddocs/plans/observability-validation/06-e2e-docs-and-release-readiness.mddocs/plans/observability-validation/07-review-remediation.mddocs/plans/observability-validation/08-diagnostic-redaction-completion.mddocs/plans/observability-validation/README.mddocs/plans/observability-validation/TODO.mddocs/plans/observability-validation/TODO/phase-08.mddocs/plans/observability-validation/gate-map.mddocs/plans/observability-validation/handoff-log-archive-phase-00-04.mddocs/plans/observability-validation/handoff-log.mddocs/plans/observability-validation/implementation-notes.mddocs/state-and-resume.mddocs/troubleshooting.mdscripts/smoke-test.shskills/bmad-story-automator/data/crash-recovery.mdskills/bmad-story-automator/data/monitoring-pattern.mdskills/bmad-story-automator/data/tmux-commands.mdskills/bmad-story-automator/src/story_automator/cli.pyskills/bmad-story-automator/src/story_automator/commands/agent_config_cmd.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_parse.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_state.pyskills/bmad-story-automator/src/story_automator/commands/state.pyskills/bmad-story-automator/src/story_automator/commands/tmux.pyskills/bmad-story-automator/src/story_automator/commands/tmux_monitor.pyskills/bmad-story-automator/src/story_automator/commands/validate_story_creation.pyskills/bmad-story-automator/src/story_automator/core/agent_config.pyskills/bmad-story-automator/src/story_automator/core/agent_config_frontmatter.pyskills/bmad-story-automator/src/story_automator/core/agent_plan.pyskills/bmad-story-automator/src/story_automator/core/diagnostics.pyskills/bmad-story-automator/src/story_automator/core/frontmatter.pyskills/bmad-story-automator/src/story_automator/core/monitoring.pyskills/bmad-story-automator/src/story_automator/core/orchestration_events.pyskills/bmad-story-automator/src/story_automator/core/parse_contracts.pyskills/bmad-story-automator/src/story_automator/core/review_verify.pyskills/bmad-story-automator/src/story_automator/core/runtime_policy.pyskills/bmad-story-automator/src/story_automator/core/session_state.pyskills/bmad-story-automator/src/story_automator/core/state_validation.pyskills/bmad-story-automator/src/story_automator/core/tmux_runtime.pyskills/bmad-story-automator/steps-c/step-02b-preflight-finalize.mdskills/bmad-story-automator/steps-v/step-v-01-check.mdtests/__init__.pytests/test_agent_config_model.pytests/test_agent_plan.pytests/test_cli_contracts.pytests/test_diagnostics.pytests/test_diagnostics_e2e.pytests/test_orchestrator_parse.pytests/test_retro_agent.pytests/test_state_policy_metadata.pytests/test_state_validation.pytests/test_stop_hooks.pytests/test_success_verifiers.pytests/test_tmux_runtime.py
Collaborator
Author
|
@augmentcode review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/bmad-story-automator/src/story_automator/commands/agent_config_cmd.py (1)
33-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate preset configs before saving.
load_presets_file()now requires every preset’sconfigto be an object, butsaveaccepts any valid JSON. Saving--config-json '"codex"'writes a presets file that laterlist,load, anddeletereject asinvalid_presets_json.Proposed fix
try: config = json.loads(config_json) except json.JSONDecodeError: print_json({"ok": False, "error": "invalid_config_json"}) return 1 + if not isinstance(config, dict): + print_json({"ok": False, "error": "invalid_config_json"}) + return 1 data = _load_presets_or_report(file_path)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/bmad-story-automator/src/story_automator/commands/agent_config_cmd.py` around lines 33 - 40, After successfully parsing the config JSON with json.loads(), add validation to ensure the parsed config is an object (dictionary) type. If the config is not a dictionary, print an error response and return 1, similar to the existing json.JSONDecodeError handling. This ensures that only object-type configs that match the requirements of load_presets_file() are saved to the presets file.
🧹 Nitpick comments (2)
docs/state-and-resume.md (1)
70-82: ⚡ Quick winConsider adding a JSON example of state-update error output.
The status-transition table and description are clear and accurate, but operators might benefit from seeing the actual JSON structure returned by
state-updatewhen an invalid transition is attempted.Example suggestion:
{ "ok": false, "error": "invalid_status_transition", "currentStatus": "READY", "attemptedStatus": "INVALID_STATE", "allowedTransitions": ["READY", "IN_PROGRESS", "PAUSED", "ABORTED"], "structuredIssues": [...] }This would make the structured diagnostics more concrete for operators.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/state-and-resume.md` around lines 70 - 82, The documentation describes how the `state-update` command rejects invalid transitions and returns structured diagnostics with currentStatus, attemptedStatus, and allowedTransitions, but does not show a concrete JSON example of this error response. Add a JSON code block example after the transition table that demonstrates the actual error response structure returned by the state-update command when an invalid transition is attempted, showing fields like ok, error, currentStatus, attemptedStatus, and allowedTransitions so operators can see the exact format they will receive.tests/test_agent_plan.py (1)
114-121: ⚡ Quick winAdd a regression case for unrelated malformed
stories[]entries.Current partial-resolution coverage doesn’t include a payload where an unrelated
stories[]entry is non-object while the requested story/task is valid. Adding that case will lock the “validate requested path only” contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_agent_plan.py` around lines 114 - 121, The current test method test_agents_plan_resolution_loader_accepts_partial_requested_task covers partial resolution but lacks a regression case where an unrelated stories[] entry is malformed (non-object) while the requested story and task remain valid. Add a new test case (either extend the existing test or create a separate one) where the agents_file payload includes a malformed unrelated story entry (for example, a string or primitive value instead of an object in the stories array) alongside the valid requested story with storyId "1.1" and task "create". Verify that load_agents_plan_for_resolution still returns an empty issues list and the correct payload for the requested path, confirming the validation only checks the requested story/task and not unrelated entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@skills/bmad-story-automator/src/story_automator/commands/orchestrator_state.py`:
- Around line 23-33: The `read_text(args[0])` call in the `state_update_action`
function can raise OSError and its subclasses (PermissionError,
IsADirectoryError, etc.) but these exceptions are not currently caught, breaking
the structured failure contract. Wrap the `read_text(args[0])` call in a
try-except block to catch OSError, and when caught, print a JSON error response
using print_json with an appropriate error message (such as "file_unreadable" or
similar) and return 1, mirroring the pattern already used for the file existence
check above it.
- Around line 83-95: The parsing loop in the state update argument handler
currently silently ignores any tokens that are not "--set" by just incrementing
the index and continuing. This allows partial updates to be applied even when
malformed input is provided. Instead of the current behavior at the end of the
while loop, detect when an unexpected token (anything other than "--set") is
encountered and return an error payload similar to what is done for duplicate
keys using state_update_duplicate_key_error_payload, ensuring that any typo or
invalid syntax causes the entire operation to fail rather than silently
processing whatever valid updates came before it.
In
`@skills/bmad-story-automator/src/story_automator/commands/validate_story_creation.py`:
- Around line 189-202: The code currently accepts any non-empty list element as
a value for --state-file and --artifacts-dir options without validating that the
value is actually a valid path (not empty, not another flag). When parsing these
options in the conditions that check idx + 1 < len(rest), add additional
validation to reject empty strings and values that start with -- (which indicate
they are other flags). If the value is invalid, invoke the error handling path
that calls print_check_error with the appropriate message, treating it the same
as when no value is provided at all.
In
`@skills/bmad-story-automator/src/story_automator/core/agent_config_frontmatter.py`:
- Around line 30-33: The validation logic in the agentConfig frontmatter parsing
currently only checks for unindented nested section keys (perTask,
complexityOverrides, retro) and raises a ValueError, but it silently skips
unindented scalar child keys like defaultPrimary, causing the configuration to
be ignored. Expand the startswith check on line 31 to include all supported
agentConfig child keys (both scalar keys like defaultPrimary and nested section
keys) so that any unindented agentConfig child key triggers the indentation
error, preventing silent configuration loss.
- Around line 83-89: The _parse_scalar function accepts malformed brace-prefixed
values like "{codex" as regular strings because they bypass the brace-matching
check. After stripping and checking for empty values, add validation logic to
reject any value that starts with "{" but does not end with "}" - either by
raising a validation error or returning an empty string to indicate rejection.
This ensures malformed frontmatter is caught as a validation error rather than
being silently accepted as an invalid identifier.
In `@skills/bmad-story-automator/src/story_automator/core/agent_plan.py`:
- Around line 267-271: The validation in the loop within
`_validate_agents_plan_resolution()` checks that all stories are objects (dict
type) before filtering for the matching story_id, which causes validation to
fail on unrelated stories. Reorganize the validation logic to first check if the
current story matches the requested story_id, and only then validate that the
matched story is a valid object type. This ensures that invalid entries in
non-matching stories do not block resolution of the target story.
In `@skills/bmad-story-automator/src/story_automator/core/diagnostics.py`:
- Around line 135-142: The `legacy_issue_message` function applies redaction via
`redact_actual()` to the message field but leaves the fallback branches
unprotected. Apply `redact_actual()` to `issue.expected`, `issue.field`, and
`issue.type` in the fallback return statements (lines with the f-string,
standalone field return, and type return) to ensure consistent redaction of
potentially sensitive values across all code paths in this function.
---
Outside diff comments:
In
`@skills/bmad-story-automator/src/story_automator/commands/agent_config_cmd.py`:
- Around line 33-40: After successfully parsing the config JSON with
json.loads(), add validation to ensure the parsed config is an object
(dictionary) type. If the config is not a dictionary, print an error response
and return 1, similar to the existing json.JSONDecodeError handling. This
ensures that only object-type configs that match the requirements of
load_presets_file() are saved to the presets file.
---
Nitpick comments:
In `@docs/state-and-resume.md`:
- Around line 70-82: The documentation describes how the `state-update` command
rejects invalid transitions and returns structured diagnostics with
currentStatus, attemptedStatus, and allowedTransitions, but does not show a
concrete JSON example of this error response. Add a JSON code block example
after the transition table that demonstrates the actual error response structure
returned by the state-update command when an invalid transition is attempted,
showing fields like ok, error, currentStatus, attemptedStatus, and
allowedTransitions so operators can see the exact format they will receive.
In `@tests/test_agent_plan.py`:
- Around line 114-121: The current test method
test_agents_plan_resolution_loader_accepts_partial_requested_task covers partial
resolution but lacks a regression case where an unrelated stories[] entry is
malformed (non-object) while the requested story and task remain valid. Add a
new test case (either extend the existing test or create a separate one) where
the agents_file payload includes a malformed unrelated story entry (for example,
a string or primitive value instead of an object in the stories array) alongside
the valid requested story with storyId "1.1" and task "create". Verify that
load_agents_plan_for_resolution still returns an empty issues list and the
correct payload for the requested path, confirming the validation only checks
the requested story/task and not unrelated entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e17b1e62-eef7-40c4-a256-276dc0da3e6f
📒 Files selected for processing (63)
docs/agents-and-monitoring.mddocs/cli-reference.mddocs/how-it-works.mddocs/plans/observability-validation/00-baseline-and-plan-reconciliation.mddocs/plans/observability-validation/01-diagnostics-contract.mddocs/plans/observability-validation/02-state-validation-and-transitions.mddocs/plans/observability-validation/03-parser-and-contract-boundaries.mddocs/plans/observability-validation/04-agent-complexity-and-story-boundaries.mddocs/plans/observability-validation/05-session-runtime-diagnostics.mddocs/plans/observability-validation/06-e2e-docs-and-release-readiness.mddocs/plans/observability-validation/07-review-remediation.mddocs/plans/observability-validation/08-diagnostic-redaction-completion.mddocs/plans/observability-validation/README.mddocs/plans/observability-validation/TODO.mddocs/plans/observability-validation/TODO/phase-08.mddocs/plans/observability-validation/gate-map.mddocs/plans/observability-validation/handoff-log-archive-phase-00-04.mddocs/plans/observability-validation/handoff-log.mddocs/plans/observability-validation/implementation-notes.mddocs/state-and-resume.mddocs/troubleshooting.mdscripts/smoke-test.shskills/bmad-story-automator/data/crash-recovery.mdskills/bmad-story-automator/data/monitoring-pattern.mdskills/bmad-story-automator/data/tmux-commands.mdskills/bmad-story-automator/src/story_automator/cli.pyskills/bmad-story-automator/src/story_automator/commands/agent_config_cmd.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_parse.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_state.pyskills/bmad-story-automator/src/story_automator/commands/state.pyskills/bmad-story-automator/src/story_automator/commands/tmux.pyskills/bmad-story-automator/src/story_automator/commands/tmux_monitor.pyskills/bmad-story-automator/src/story_automator/commands/validate_story_creation.pyskills/bmad-story-automator/src/story_automator/core/agent_config.pyskills/bmad-story-automator/src/story_automator/core/agent_config_frontmatter.pyskills/bmad-story-automator/src/story_automator/core/agent_plan.pyskills/bmad-story-automator/src/story_automator/core/diagnostics.pyskills/bmad-story-automator/src/story_automator/core/frontmatter.pyskills/bmad-story-automator/src/story_automator/core/monitoring.pyskills/bmad-story-automator/src/story_automator/core/orchestration_events.pyskills/bmad-story-automator/src/story_automator/core/parse_contracts.pyskills/bmad-story-automator/src/story_automator/core/review_verify.pyskills/bmad-story-automator/src/story_automator/core/runtime_policy.pyskills/bmad-story-automator/src/story_automator/core/session_state.pyskills/bmad-story-automator/src/story_automator/core/state_validation.pyskills/bmad-story-automator/src/story_automator/core/tmux_runtime.pyskills/bmad-story-automator/steps-c/step-02b-preflight-finalize.mdskills/bmad-story-automator/steps-v/step-v-01-check.mdtests/__init__.pytests/test_agent_config_model.pytests/test_agent_plan.pytests/test_cli_contracts.pytests/test_diagnostics.pytests/test_diagnostics_e2e.pytests/test_orchestrator_parse.pytests/test_retro_agent.pytests/test_state_policy_metadata.pytests/test_state_validation.pytests/test_stop_hooks.pytests/test_success_verifiers.pytests/test_tmux_runtime.py
Comment on lines
+23
to
+33
| def state_update_action(args: list[str]) -> int: | ||
| if not args or not file_exists(args[0]): | ||
| print_json({"ok": False, "error": "file_not_found"}) | ||
| return 1 | ||
| text = read_text(args[0]) | ||
| frontmatter, body = _split_frontmatter(text) | ||
| fields = parse_frontmatter_content(_frontmatter_content(frontmatter)) | ||
| updates = _parse_updates(args[1:]) | ||
| if isinstance(updates, dict): | ||
| print_json(updates) | ||
| return 1 |
There was a problem hiding this comment.
Handle unreadable state files with controlled JSON failures.
Line 27 can raise OSError (PermissionError, IsADirectoryError, etc.) and currently bypasses your structured failure contract.
Suggested fix
def state_update_action(args: list[str]) -> int:
if not args or not file_exists(args[0]):
print_json({"ok": False, "error": "file_not_found"})
return 1
- text = read_text(args[0])
+ try:
+ text = read_text(args[0])
+ except OSError as exc:
+ issues = issues_from_exception(exc, source="state-update", field="state-file")
+ print_json(
+ {
+ "ok": False,
+ "error": "file_read_failed",
+ "issues": [str(redact_actual(legacy_issue_message(issue))) for issue in issues],
+ "structuredIssues": serialize_issues(issues),
+ }
+ )
+ return 1
frontmatter, body = _split_frontmatter(text)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@skills/bmad-story-automator/src/story_automator/commands/orchestrator_state.py`
around lines 23 - 33, The `read_text(args[0])` call in the `state_update_action`
function can raise OSError and its subclasses (PermissionError,
IsADirectoryError, etc.) but these exceptions are not currently caught, breaking
the structured failure contract. Wrap the `read_text(args[0])` call in a
try-except block to catch OSError, and when caught, print a JSON error response
using print_json with an appropriate error message (such as "file_unreadable" or
similar) and return 1, mirroring the pattern already used for the file existence
check above it.
Comment on lines
+83
to
+95
| while idx < len(args): | ||
| if args[idx] == "--set": | ||
| parsed = parse_state_update_argument(args[idx + 1] if idx + 1 < len(args) else "") | ||
| if isinstance(parsed, dict): | ||
| return parsed | ||
| if parsed[0] in seen: | ||
| return state_update_duplicate_key_error_payload(parsed[0]) | ||
| seen.add(parsed[0]) | ||
| updates.append(parsed) | ||
| idx += 2 | ||
| continue | ||
| idx += 1 | ||
| return updates |
There was a problem hiding this comment.
Reject unsupported tokens in update parsing to avoid partial-intent writes.
Lines 83-95 silently ignore non---set tokens. A typo can still apply some updates and hide the malformed input.
Suggested fix
def _parse_updates(args: list[str]) -> list[tuple[str, str]] | dict[str, object]:
updates: list[tuple[str, str]] = []
seen: set[str] = set()
idx = 0
while idx < len(args):
- if args[idx] == "--set":
+ token = args[idx]
+ if token == "--set":
parsed = parse_state_update_argument(args[idx + 1] if idx + 1 < len(args) else "")
if isinstance(parsed, dict):
return parsed
if parsed[0] in seen:
return state_update_duplicate_key_error_payload(parsed[0])
seen.add(parsed[0])
updates.append(parsed)
idx += 2
continue
- idx += 1
+ return {"ok": False, "error": "unsupported_argument", "argument": token}
return updates🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@skills/bmad-story-automator/src/story_automator/commands/orchestrator_state.py`
around lines 83 - 95, The parsing loop in the state update argument handler
currently silently ignores any tokens that are not "--set" by just incrementing
the index and continuing. This allows partial updates to be applied even when
malformed input is provided. Instead of the current behavior at the end of the
while loop, detect when an unexpected token (anything other than "--set") is
encountered and return an error payload similar to what is done for duplicate
keys using state_update_duplicate_key_error_payload, ensuring that any typo or
invalid syntax causes the entire operation to fail rather than silently
processing whatever valid updates came before it.
Comment on lines
189
to
+202
| if rest[idx] == "--artifacts-dir" and idx + 1 < len(rest): | ||
| artifacts_dir = Path(rest[idx + 1]) | ||
| idx += 2 | ||
| continue | ||
| if rest[idx] == "--artifacts-dir": | ||
| before_count, after_count = parsed_delta_counts(before_value, after_value) | ||
| return print_check_error(story_id, reason="--artifacts-dir requires a value", before_count=before_count, after_count=after_count) | ||
| return print_check_error(story_id, reason="--artifacts-dir requires a value", field="--artifacts-dir", before_count=before_count, after_count=after_count) | ||
| if rest[idx] == "--state-file" and idx + 1 < len(rest): | ||
| state_file = rest[idx + 1] | ||
| idx += 2 | ||
| continue | ||
| if rest[idx] == "--state-file": | ||
| before_count, after_count = parsed_delta_counts(before_value, after_value) | ||
| return print_check_error(story_id, reason="--state-file requires a value", before_count=before_count, after_count=after_count) | ||
| return print_check_error(story_id, reason="--state-file requires a value", field="--state-file", before_count=before_count, after_count=after_count) |
There was a problem hiding this comment.
Reject empty or flag-like values before accepting path options.
check --state-file "" currently sets state_file to an empty string, then create_check_payload() falls back to None, so validation runs against live policy instead of the requested snapshot. --state-file --before / --artifacts-dir --state-file also get consumed as values and produce misleading downstream errors.
Proposed fix
- if rest[idx] == "--artifacts-dir" and idx + 1 < len(rest):
- artifacts_dir = Path(rest[idx + 1])
- idx += 2
- continue
if rest[idx] == "--artifacts-dir":
+ if idx + 1 >= len(rest) or not rest[idx + 1].strip() or rest[idx + 1].startswith("--"):
+ before_count, after_count = parsed_delta_counts(before_value, after_value)
+ return print_check_error(story_id, reason="--artifacts-dir requires a value", field="--artifacts-dir", before_count=before_count, after_count=after_count)
+ artifacts_dir = Path(rest[idx + 1])
+ idx += 2
+ continue
- before_count, after_count = parsed_delta_counts(before_value, after_value)
- return print_check_error(story_id, reason="--artifacts-dir requires a value", field="--artifacts-dir", before_count=before_count, after_count=after_count)
- if rest[idx] == "--state-file" and idx + 1 < len(rest):
- state_file = rest[idx + 1]
- idx += 2
- continue
if rest[idx] == "--state-file":
+ if idx + 1 >= len(rest) or not rest[idx + 1].strip() or rest[idx + 1].startswith("--"):
+ before_count, after_count = parsed_delta_counts(before_value, after_value)
+ return print_check_error(story_id, reason="--state-file requires a value", field="--state-file", before_count=before_count, after_count=after_count)
+ state_file = rest[idx + 1]
+ idx += 2
+ continue
- before_count, after_count = parsed_delta_counts(before_value, after_value)
- return print_check_error(story_id, reason="--state-file requires a value", field="--state-file", before_count=before_count, after_count=after_count)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@skills/bmad-story-automator/src/story_automator/commands/validate_story_creation.py`
around lines 189 - 202, The code currently accepts any non-empty list element as
a value for --state-file and --artifacts-dir options without validating that the
value is actually a valid path (not empty, not another flag). When parsing these
options in the conditions that check idx + 1 < len(rest), add additional
validation to reject empty strings and values that start with -- (which indicate
they are other flags). If the value is invalid, invoke the error handling path
that calls print_check_error with the appropriate message, treating it the same
as when no value is provided at all.
Comment on lines
+30
to
+33
| if raw_line and not raw_line.startswith(" "): | ||
| if raw_line.strip().startswith(("perTask:", "complexityOverrides:", "retro:")): | ||
| raise ValueError("agentConfig nested sections must be indented") | ||
| break |
There was a problem hiding this comment.
Reject unindented scalar agentConfig child keys too.
agentConfig:\ndefaultPrimary: "codex" currently hits Line 30, breaks the block, and returns {}, so the intended runtime config is silently ignored and defaults to auto. Treat all supported agentConfig child keys as indentation errors here, not just nested sections.
Proposed fix
- if raw_line and not raw_line.startswith(" "):
- if raw_line.strip().startswith(("perTask:", "complexityOverrides:", "retro:")):
+ if raw_line and not raw_line.startswith(" "):
+ if raw_line.strip().startswith(
+ (
+ "defaultPrimary:",
+ "primary:",
+ "defaultFallback:",
+ "fallback:",
+ "defaultModel:",
+ "perTask:",
+ "complexityOverrides:",
+ "retro:",
+ "low:",
+ "medium:",
+ "high:",
+ )
+ ):
raise ValueError("agentConfig nested sections must be indented")
break🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@skills/bmad-story-automator/src/story_automator/core/agent_config_frontmatter.py`
around lines 30 - 33, The validation logic in the agentConfig frontmatter
parsing currently only checks for unindented nested section keys (perTask,
complexityOverrides, retro) and raises a ValueError, but it silently skips
unindented scalar child keys like defaultPrimary, causing the configuration to
be ignored. Expand the startswith check on line 31 to include all supported
agentConfig child keys (both scalar keys like defaultPrimary and nested section
keys) so that any unindented agentConfig child key triggers the indentation
error, preventing silent configuration loss.
Comment on lines
+83
to
+89
| def _parse_scalar(raw: str) -> object: | ||
| value = _strip_inline_yaml_comment(raw).strip() | ||
| if not value: | ||
| return "" | ||
| if value.startswith("{") and value.endswith("}"): | ||
| return _parse_inline_map(value) | ||
| value = _unquote_checked(value) |
There was a problem hiding this comment.
Reject malformed brace-prefixed scalar values.
A value like defaultPrimary: {codex skips _parse_inline_map() and is accepted as a non-empty string, which turns malformed frontmatter into an invalid agent/model identifier instead of a validation error.
Proposed fix
- if value.startswith("{") and value.endswith("}"):
- return _parse_inline_map(value)
+ if value.startswith("{") or value.endswith("}"):
+ if not (value.startswith("{") and value.endswith("}")):
+ raise ValueError("agentConfig inline maps must have balanced braces")
+ return _parse_inline_map(value)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@skills/bmad-story-automator/src/story_automator/core/agent_config_frontmatter.py`
around lines 83 - 89, The _parse_scalar function accepts malformed
brace-prefixed values like "{codex" as regular strings because they bypass the
brace-matching check. After stripping and checking for empty values, add
validation logic to reject any value that starts with "{" but does not end with
"}" - either by raising a validation error or returning an empty string to
indicate rejection. This ensures malformed frontmatter is caught as a validation
error rather than being silently accepted as an invalid identifier.
Comment on lines
+267
to
+271
| for index, story in enumerate(stories): | ||
| field = f"stories[{index}]" | ||
| if not isinstance(story, dict): | ||
| return [_issue("invalid_type", field, "object", story, "Agents plan story must be an object")] | ||
| if story.get("storyId") != story_id: |
There was a problem hiding this comment.
Validate only the matched story during resolution pre-checks.
On Line 269, _validate_agents_plan_resolution() returns invalid_type for any non-object entry in stories, even when that entry is unrelated to the requested story_id. That makes agents-resolve fail for partial user-authored files where the target story/task is valid.
Proposed fix
def _validate_agents_plan_resolution(payload: dict[str, Any], story_id: str, task: str) -> list[DiagnosticIssue]:
if task not in TASKS:
return [_issue("unsupported_task", "task", sorted(TASKS), task, f"{task} task is not supported")]
stories = payload.get("stories") or []
for index, story in enumerate(stories):
field = f"stories[{index}]"
- if not isinstance(story, dict):
- return [_issue("invalid_type", field, "object", story, "Agents plan story must be an object")]
+ if not isinstance(story, dict):
+ continue
if story.get("storyId") != story_id:
continue
tasks = story.get("tasks")
if not isinstance(tasks, dict):
return [_issue("invalid_type", f"{field}.tasks", "object", tasks, "Agents plan tasks must be an object")]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for index, story in enumerate(stories): | |
| field = f"stories[{index}]" | |
| if not isinstance(story, dict): | |
| return [_issue("invalid_type", field, "object", story, "Agents plan story must be an object")] | |
| if story.get("storyId") != story_id: | |
| for index, story in enumerate(stories): | |
| field = f"stories[{index}]" | |
| if not isinstance(story, dict): | |
| continue | |
| if story.get("storyId") != story_id: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/bmad-story-automator/src/story_automator/core/agent_plan.py` around
lines 267 - 271, The validation in the loop within
`_validate_agents_plan_resolution()` checks that all stories are objects (dict
type) before filtering for the matching story_id, which causes validation to
fail on unrelated stories. Reorganize the validation logic to first check if the
current story matches the requested story_id, and only then validate that the
matched story is a valid object type. This ensures that invalid entries in
non-matching stories do not block resolution of the target story.
Comment on lines
+135
to
+142
| def legacy_issue_message(issue: DiagnosticIssue) -> str: | ||
| if issue.message: | ||
| return str(redact_actual(issue.message)) | ||
| if issue.field and issue.expected: | ||
| return f"{issue.field}: expected {issue.expected}" | ||
| if issue.field: | ||
| return issue.field | ||
| return issue.type |
There was a problem hiding this comment.
Redact expected/field in legacy fallback messages.
legacy_issue_message() still returns raw issue.expected/issue.field in fallback branches, which can bypass the redaction contract and leak sensitive values in legacy text outputs.
Proposed patch
def legacy_issue_message(issue: DiagnosticIssue) -> str:
if issue.message:
return str(redact_actual(issue.message))
if issue.field and issue.expected:
- return f"{issue.field}: expected {issue.expected}"
+ safe_field = str(redact_actual(issue.field))
+ safe_expected = redact_actual(_json_safe(issue.expected))
+ return f"{safe_field}: expected {safe_expected}"
if issue.field:
- return issue.field
+ return str(redact_actual(issue.field))
return issue.type📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def legacy_issue_message(issue: DiagnosticIssue) -> str: | |
| if issue.message: | |
| return str(redact_actual(issue.message)) | |
| if issue.field and issue.expected: | |
| return f"{issue.field}: expected {issue.expected}" | |
| if issue.field: | |
| return issue.field | |
| return issue.type | |
| def legacy_issue_message(issue: DiagnosticIssue) -> str: | |
| if issue.message: | |
| return str(redact_actual(issue.message)) | |
| if issue.field and issue.expected: | |
| safe_field = str(redact_actual(issue.field)) | |
| safe_expected = str(redact_actual(issue.expected)) | |
| return f"{safe_field}: expected {safe_expected}" | |
| if issue.field: | |
| return str(redact_actual(issue.field)) | |
| return issue.type |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/bmad-story-automator/src/story_automator/core/diagnostics.py` around
lines 135 - 142, The `legacy_issue_message` function applies redaction via
`redact_actual()` to the message field but leaves the fallback branches
unprotected. Apply `redact_actual()` to `issue.expected`, `issue.field`, and
`issue.type` in the fallback return statements (lines with the f-string,
standalone field return, and type return) to ensure consistent redaction of
potentially sensitive values across all code paths in this function.
|
Sorry, Augment failed to review this pull request. |
Collaborator
Author
|
@augmentcode review |
|
Sorry, Augment failed to review this pull request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Draft PR for the observability and validation work tracked under issue #5.
This branch makes automator failures easier to diagnose across parser, state, agent-plan, and tmux/session boundaries while preserving successful CLI behavior and legacy output fields. The implementation follows the observability-validation plan: small typed/domain seams, additive diagnostics, focused tests, and no broad OO rewrite.
What Changed
Diagnostics contract
typefieldexpectedactualmessagerecoverycodeseveritysourcestructuredIssuesonly beside legacy fields where applicable.State validation and transitions
validate-statefields:ok,structure, and legacyissuesstrings.structuredIssuesandissueCounttovalidate-state.state-update.Parser and verifier contracts
reasonvalues and addedstructuredIssueson failures.Agent plan and complexity validation
agents-buildnow fails early with structured diagnostics for malformed complexity/config payloads.agents-resolvevalidates only the requested story/task path for direct resolution, preserving compatibility with partial user-authored agents files.Session/runtime diagnostics
core/session_state.pymodule.load_session_state()behavior returning{}for missing/unreadable/invalid/non-object state.monitor-session --jsonnow reports malformed persisted session-state diagnostics when relevant.CLI/tmux contract fixes
parse-story-rangeinvalid totals to return stable JSON rather than raising before the error handler.parse-storyerror reporting for missing rules versus invalid rules JSON.tmux-wrapper name --cycle Nto use the cycle value, not the flag token.tmux-wrapper name review 5 5.3 2.kill-allproject-scoped by default, with--all-projectsas explicit opt-in.Docs and plan hygiene
project-slugandproject-hash.Compatibility Notes
validate-statekeeps legacyissues: list[str]and addsstructuredIssuesplusissueCount.status: "error"andreasonfields.monitor-session --jsonpreserves existing fields and addsstructuredIssuesonly when relevant.agents-resolveremains compatible with partial plans containing the requested story/task.Review Findings Addressed
This branch includes fixes from a staged subagent review loop:
state-update.agents-resolverejecting otherwise resolvable partial direct agents files.tmux-wrapper name.Latest review baseline: P0/P1 clean.
Verification
Commands run locally:
Results:
263 tests OK.npm run verifypassed.bmad-qa-generate-e2e-testswarnings only.Known Gaps / Risks
core/tmux_runtime.pyandcore/runtime_policy.py.Refs #5
Summary by CodeRabbit
New Features
structuredIssues+issueCount) alongside legacy fields, plus opt-in newline JSON diagnostic events.statustransition rules with structured invalid-transition reporting.monitor-session --jsonsession-state diagnostics; tmuxkill-allnow supports multi-project scope.Bug Fixes
Documentation
Tests