feat: add Codex runtime support#3
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:
WalkthroughThis PR generalizes runtime and installation paths beyond Claude to support multiple providers (Claude/Codex/.agents), makes retrospectives use the configured retro agent by default, implements provider-aware stop-hook installers (Claude JSON and Codex TOML), adds runtime-layout and marker resolution utilities, extends orchestrator helper actions, updates workflow steps and docs, and adds tests for layout, stop-hooks, and retro-agent resolution. ChangesRuntime-aware retrospectives, marker handling, and stop-hook support
Sequence DiagramsequenceDiagram
participant User as User/Orchestrator
participant Script as Workflow Script
participant Helper as orchestrator-helper
participant Layout as runtime_layout
participant StopHooks as stop_hooks
participant Provider as Claude/Codex
User->>Script: start workflow
Script->>Helper: orchestrator-helper marker path
Helper->>Layout: active_marker_path()
Layout-->>Helper: {file, entry}
Helper-->>Script: JSON {file,entry}
Script->>Script: ensure-marker-gitignore (entry)
Script->>Helper: orchestrator-helper retro-agent --state-file
Helper->>Helper: load agentConfig from state/frontmatter
Helper->>Layout: runtime_provider()
Helper-->>Script: {primary,fallback} for retro
Script->>Script: tmux-wrapper build-cmd/spawn using resolved agent
Script->>Helper: orchestrator-helper marker create
Helper->>Layout: active_marker_path()
Helper->>Provider: write marker at resolved location
alt Install / Ensure Stop Hook
Script->>Helper: orchestrator-helper ensure-stop-hook
Helper->>StopHooks: ensure_stop_hook(provider=...)
StopHooks->>Provider: update provider-specific config (JSON/TOML)
StopHooks-->>Helper: {changed,reason,...}
end
Script->>Helper: orchestrator-helper marker remove
Helper->>Layout: active_marker_path()
Helper->>Provider: remove marker
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
dickymoore
force-pushed
the
dicky/codex-runtime-support
branch
from
7707edc to
d07c0ff
Compare
🤖 Augment PR SummarySummary: This PR adds first-class Codex runtime layout support to Story Automator so installers/consumers no longer need to patch paths and hook behavior post-install. Changes:
Technical Notes: Provider selection can be forced via env vars (e.g., 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
source/src/story_automator/commands/orchestrator_epic_agents.py (1)
233-331: 💤 Low valueConsider documenting the expected YAML structure.
This custom indentation-based YAML parser handles the
agentConfigfrontmatter section with specific indent levels (2/4/6/8 spaces). The implementation appears correct for the expected structure, but the tight coupling to specific indentation could be fragile if the frontmatter format evolves.The final
parse_agent_config(json.dumps(config))call ensures consistent post-processing, which is good.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/src/story_automator/commands/orchestrator_epic_agents.py` around lines 233 - 331, The _load_agent_config_from_state function uses a custom indentation-based parser for the agentConfig frontmatter and is tightly coupled to exact indent levels (2/4/6/8) and keys like "agentConfig:", "perTask:", "complexityOverrides:", and "retro:"—document the expected frontmatter schema and indentation rules immediately above the _load_agent_config_from_state definition (or as its docstring) describing required top-level marker "agentConfig", the allowed child sections ("perTask", "complexityOverrides", "retro"), the precise indent levels for section headers (2 spaces) and nested keys (4/6/8 spaces), and example snippets for each structure so future authors know how to format input and what the parser expects; also mention that parse_agent_config(json.dumps(config)) is applied to the constructed dict for post-processing so callers can rely on that normalization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@payload/.claude/skills/bmad-story-automator/data/stop-hook-troubleshooting.md`:
- Around line 113-114: The troubleshooting hint in stop-hook-troubleshooting.md
hardcodes the path ".claude/skills/bmad-story-automator/scripts/" which misleads
users on .agents/.codex runtimes; update the table entry to mention and check
all possible runtime skill trees (e.g., .claude, .agents, .codex) or provide a
single command that checks each location (for example suggest running ls -la
against .claude/skills/... OR .agents/skills/... OR .codex/skills/...) and/or
recommend using a glob like */skills/bmad-story-automator/scripts/ so the
diagnostic works for Codex and Agents layouts; change the exact hardcoded path
string in the table row to the more general variants so the troubleshooting flow
no longer assumes only the .claude layout.
In `@source/tests/test_stop_hooks.py`:
- Line 224: The S108 linter flag on the assertion accessing
config["projects"]["/tmp/example"] is a false positive because "/tmp/example" is
a TOML table key in the test fixture, so suppress the warning by adding a
per-line noqa: S108 comment to the assertion containing
config["projects"]["/tmp/example"] (the self.assertEqual(... trust_level ...)
line) so the linter ignores it without changing test semantics.
- Around line 139-147: The test asserts changed=False even though normalization
mutates hooks.json (timeout/statusMessage), which is confusing; update the
code/tests to clarify the semantic contract: either add a concise inline comment
in source/tests/test_stop_hooks.py next to the _run_ensure_stop_hook("codex")
assertion explaining that changed==True denotes a newly registered hook
requiring restart while normalization-only writes still return changed==False,
or update the production cmd_ensure_stop_hook function/method
docstring/return-schema to explicitly state that normalization writes do not set
changed=True; reference the symbols _run_ensure_stop_hook and
cmd_ensure_stop_hook (and the hooks.json normalization behavior) when adding the
comment or docstring so readers can locate the behavior.
---
Nitpick comments:
In `@source/src/story_automator/commands/orchestrator_epic_agents.py`:
- Around line 233-331: The _load_agent_config_from_state function uses a custom
indentation-based parser for the agentConfig frontmatter and is tightly coupled
to exact indent levels (2/4/6/8) and keys like "agentConfig:", "perTask:",
"complexityOverrides:", and "retro:"—document the expected frontmatter schema
and indentation rules immediately above the _load_agent_config_from_state
definition (or as its docstring) describing required top-level marker
"agentConfig", the allowed child sections ("perTask", "complexityOverrides",
"retro"), the precise indent levels for section headers (2 spaces) and nested
keys (4/6/8 spaces), and example snippets for each structure so future authors
know how to format input and what the parser expects; also mention that
parse_agent_config(json.dumps(config)) is applied to the constructed dict for
post-processing so callers can rely on that normalization.
🪄 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: 0d781ed9-54d7-48a6-8796-b9dadd96fcf8
📒 Files selected for processing (30)
README.mddocs/agents-and-monitoring.mddocs/story-execution.mdpayload/.claude/skills/bmad-story-automator/data/agent-config-prompts.mdpayload/.claude/skills/bmad-story-automator/data/marker-file-format.mdpayload/.claude/skills/bmad-story-automator/data/orchestrator-rules-appendix.mdpayload/.claude/skills/bmad-story-automator/data/retrospective-automation.mdpayload/.claude/skills/bmad-story-automator/data/scripts-reference.mdpayload/.claude/skills/bmad-story-automator/data/stop-hook-config.mdpayload/.claude/skills/bmad-story-automator/data/stop-hook-recovery.mdpayload/.claude/skills/bmad-story-automator/data/stop-hook-troubleshooting.mdpayload/.claude/skills/bmad-story-automator/data/tmux-long-command-testing.mdpayload/.claude/skills/bmad-story-automator/steps-c/step-01-init.mdpayload/.claude/skills/bmad-story-automator/steps-c/step-01b-continue.mdpayload/.claude/skills/bmad-story-automator/steps-c/step-02a-preflight-config.mdpayload/.claude/skills/bmad-story-automator/steps-c/step-02b-preflight-finalize.mdpayload/.claude/skills/bmad-story-automator/steps-c/step-03b-execute-finish.mdpayload/.claude/skills/bmad-story-automator/steps-c/step-04-wrapup.mdpayload/.claude/skills/bmad-story-automator/workflow.mdsource/src/story_automator/commands/basic.pysource/src/story_automator/commands/orchestrator.pysource/src/story_automator/commands/orchestrator_epic_agents.pysource/src/story_automator/commands/state.pysource/src/story_automator/core/agent_config.pysource/src/story_automator/core/runtime_layout.pysource/src/story_automator/core/runtime_policy.pysource/src/story_automator/core/stop_hooks.pysource/tests/test_runtime_layout.pysource/tests/test_state_policy_metadata.pysource/tests/test_stop_hooks.py
💤 Files with no reviewable changes (1)
- payload/.claude/skills/bmad-story-automator/steps-c/step-02a-preflight-config.md
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/installation-and-layout.md (1)
22-27: ⚡ Quick winClarify Codex path split between skill root and hook/config root.
This doc correctly uses
.agents/skills/...for installed skills, but it should explicitly note that Codex stop-hook/config files are under.codex/...to avoid operator confusion when wiring runtime behavior.Proposed doc tweak
The installer writes into the target project's native runtime skill root: - Claude Code: `.claude/skills/bmad-story-automator` - Claude Code: `.claude/skills/bmad-story-automator-review` - Codex: `.agents/skills/bmad-story-automator` - Codex: `.agents/skills/bmad-story-automator-review` + +Note: for Codex, skill assets live under `.agents/skills/...`, while hook/config files are managed under `.codex/...`.Also applies to: 88-93
🤖 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/installation-and-layout.md` around lines 22 - 27, Update the installation-and-layout.md section that lists install paths to explicitly distinguish Codex runtime skill roots from Codex hook/config roots: keep the installed skill paths as `.agents/skills/bmad-story-automator` and `.agents/skills/bmad-story-automator-review` but add a clarifying sentence that Codex stop-hook and configuration files live under `.codex/...` (e.g., `.codex/.../bmad-story-automator`), and mirror the same clarification for the repeated mention at lines ~88-93 so readers understand where to wire runtime behavior versus hook/config files.source/src/story_automator/core/stop_hooks.py (1)
352-365: 💤 Low valueOptional: preserve a space before the trailing comment.
When rewriting an inline
features = { ... } # comment, the regex consumes the whitespace between}and#, and the formatter then concatenatescommentdirectly, producing}# comment. It's still valid TOML and the tests pass, but a single space would keep the file visually identical to common formatting.♻️ Suggested cosmetic tweak
- indent, inner, comment = match.group(1), match.group(2), match.group(3) or "" + indent, inner, comment = match.group(1), match.group(2), match.group(3) or "" + if comment: + comment = f" {comment}" @@ - return f"{indent}features = {{ {', '.join(updated_items)} }}{comment}" + return f"{indent}features = {{ {', '.join(updated_items)} }}{comment}"🤖 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 `@source/src/story_automator/core/stop_hooks.py` around lines 352 - 365, The function _set_inline_features_table_line currently drops the space between the closing brace and a trailing comment because match.group(3) consumes the intervening whitespace; update it so when a comment exists you prepend a single space before appending it. Locate _set_inline_features_table_line, keep the existing item parsing and replacement logic, and change how comment is built/appended (e.g., if match.group(3) is truthy ensure comment starts with a single space before the "#" token) so the returned string uses " ... }}<space># comment" instead of " ... }}# comment".
🤖 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 `@docs/installation-and-layout.md`:
- Around line 22-27: Update the installation-and-layout.md section that lists
install paths to explicitly distinguish Codex runtime skill roots from Codex
hook/config roots: keep the installed skill paths as
`.agents/skills/bmad-story-automator` and
`.agents/skills/bmad-story-automator-review` but add a clarifying sentence that
Codex stop-hook and configuration files live under `.codex/...` (e.g.,
`.codex/.../bmad-story-automator`), and mirror the same clarification for the
repeated mention at lines ~88-93 so readers understand where to wire runtime
behavior versus hook/config files.
In `@source/src/story_automator/core/stop_hooks.py`:
- Around line 352-365: The function _set_inline_features_table_line currently
drops the space between the closing brace and a trailing comment because
match.group(3) consumes the intervening whitespace; update it so when a comment
exists you prepend a single space before appending it. Locate
_set_inline_features_table_line, keep the existing item parsing and replacement
logic, and change how comment is built/appended (e.g., if match.group(3) is
truthy ensure comment starts with a single space before the "#" token) so the
returned string uses " ... }}<space># comment" instead of " ... }}# comment".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdbfc780-04f5-4981-a66b-25afd01ac783
📒 Files selected for processing (14)
README.mddocs/agents-and-monitoring.mddocs/cli-reference.mddocs/how-it-works.mddocs/installation-and-layout.mddocs/troubleshooting.mdpayload/.claude/skills/bmad-story-automator/data/stop-hook-troubleshooting.mdsource/src/story_automator/commands/basic.pysource/src/story_automator/commands/orchestrator_epic_agents.pysource/src/story_automator/core/runtime_layout.pysource/src/story_automator/core/stop_hooks.pysource/tests/test_runtime_layout.pysource/tests/test_state_policy_metadata.pysource/tests/test_stop_hooks.py
✅ Files skipped from review due to trivial changes (5)
- docs/cli-reference.md
- docs/troubleshooting.md
- docs/how-it-works.md
- README.md
- docs/agents-and-monitoring.md
🚧 Files skipped from review as they are similar to previous changes (5)
- source/tests/test_runtime_layout.py
- payload/.claude/skills/bmad-story-automator/data/stop-hook-troubleshooting.md
- source/tests/test_state_policy_metadata.py
- source/src/story_automator/core/runtime_layout.py
- source/src/story_automator/commands/basic.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@source/src/story_automator/core/stop_hooks.py`:
- Around line 352-365: The function _set_inline_features_table_line is losing
the space between the closing brace and a trailing inline comment because
match.group(3) contains the comment token "#..." but not the separator space;
when rebuilding the line ensure you preserve that separator by prefixing a
single space when a trailing comment exists (e.g., compute comment =
match.group(3) or "" and then use comment = (" " + comment) if comment and not
comment.startswith(" ") else comment) before returning the formatted string so
features = {{ ... }} # comment keeps its original space; update only
_set_inline_features_table_line accordingly (no regex change required) and keep
returned format f"{indent}features = {{ {', '.join(updated_items)} }}{comment}".
- Around line 256-259: Remove the unused wrapper function
_ensure_codex_hooks_feature: delete its definition (the function that calls
_prepare_codex_hooks_feature, then _write_prepared_update, and returns
update.result) because ensure_codex_stop_hook already calls
_prepare_codex_hooks_feature and handles writes directly; ensure any imports or
references to _ensure_codex_hooks_feature are also removed so there are no
dangling references, and run tests to verify nothing else depends on
_ensure_codex_hooks_feature.
🪄 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: 585e2367-a0f2-4043-a187-6c01c31867a2
📒 Files selected for processing (8)
README.mddocs/agents-and-monitoring.mddocs/cli-reference.mddocs/how-it-works.mddocs/installation-and-layout.mddocs/troubleshooting.mdsource/src/story_automator/core/stop_hooks.pysource/tests/test_stop_hooks.py
✅ Files skipped from review due to trivial changes (5)
- docs/agents-and-monitoring.md
- docs/cli-reference.md
- README.md
- docs/how-it-works.md
- docs/installation-and-layout.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/troubleshooting.md
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/src/story_automator/commands/state.py (1)
240-251:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
validate-statestill requiresaiCommand, contradicting the new deprecated/empty default.The state template now defaults
aiCommandto""(deprecated, replaced byagentConfig), andcmd_build_state_docno longer enforces a non-empty value. However,cmd_validate_statestill callsrequired("aiCommand"), which flags any empty string as"Missing or empty aiCommand". State documents generated under the new agentConfig-only flow will therefore fail validation even though they are correct.Consider relaxing the check so
aiCommandis optional whenagentConfigis present (or drop the requirement entirely since the field is documented as deprecated).🛡️ Possible adjustment
required("status", lambda value: isinstance(value, str) and value in allowed) required("lastUpdated", lambda value: isinstance(value, str) and re.search(r"\d{4}-\d{2}-\d{2}T", value)) - required("aiCommand") + # aiCommand is deprecated in favor of agentConfig; only required if agentConfig is absent. + if not fields.get("agentConfig"): + required("aiCommand")🤖 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 `@source/src/story_automator/commands/state.py` around lines 240 - 251, The validator currently calls required("aiCommand") and flags empty strings even though aiCommand is deprecated; update the validation in the state validation function (the block that calls required(...) and load_policy_for_state) to make aiCommand optional when agentConfig is present: replace the unconditional required("aiCommand") with a conditional check that only requires a non-empty aiCommand if state.get("agentConfig") is falsy (or drop the requirement entirely if you prefer); ensure any existing error messages still use the required(...) helper for consistency when the check is enforced.source/src/story_automator/commands/tmux.py (1)
124-135:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve the default agent after
rootis known.
agent_type()now returns a concrete provider, so theseif agent == "auto"branches only run for an explicit--agent auto. In the default path, provider inference still happens beforeget_project_root()is available, which means invoking the wrapper from outside the repo can still pick the wrong runtime. Start with"auto"/the raw env value and resolve it withruntime_provider(root)oncerootis computed.Also applies to: 174-178
🤖 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 `@source/src/story_automator/commands/tmux.py` around lines 124 - 135, The code currently calls agent_type() and then later may override it with "--agent", causing provider inference to run before get_project_root(); change the flow so agent is initialized to the raw marker (e.g., "auto" or the raw env value) instead of calling agent_type() up front, parse args to possibly set agent string, then compute root = get_project_root() and only then, if agent == "auto" (or equals the raw env placeholder), call runtime_provider(root) to get the concrete provider; apply the same change to the second occurrence around the block referenced at lines 174-178 (replace upfront agent_type() calls with the raw marker and resolve via runtime_provider(root) after root is known).
🧹 Nitpick comments (2)
payload/.claude/skills/bmad-story-automator/data/tmux-commands.md (1)
80-80: ⚡ Quick winUse consistent placeholder syntax and document the values.
The placeholder syntax mixes
{project_root}(curly braces) with<installed-skill-root>(angle brackets), which is inconsistent with the rest of the document that uses curly braces for all placeholders (e.g.,{epic},{story_suffix},{SESSION_NAME}).Additionally, there's no explanation of what value users should substitute for
<installed-skill-root>. Users won't know whether to use.claude/skills,.codex/skills, or something else.Consider:
- Using consistent curly-brace syntax:
{installed-skill-root}- Adding a note explaining the expected values for Claude vs Codex, or
- Providing a shell variable example showing how to determine it dynamically
🤖 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 `@payload/.claude/skills/bmad-story-automator/data/tmux-commands.md` at line 80, Replace the inconsistent placeholder in the tmux command assignment (the script variable name "script") by using {installed-skill-root} instead of <installed-skill-root>, update the line to match the document's curly-brace convention (e.g., "{project_root}/ {installed-skill-root}/bmad-story-automator/..."), and add a short note near that line describing allowable values (for example: use ".claude/skills" for Claude, ".codex/skills" for Codex) plus an optional shell example hint for users to derive it dynamically (e.g., show how to detect the skill root via an environment variable or a simple shell substitution).source/src/story_automator/commands/state.py (1)
80-93: 💤 Low valueDefensive check on
agent_config.get('defaultFallback')returningNone.
agent_config.get('defaultFallback', agent_config.get('fallback', False))only triggers the default when the key is absent. If a user (or earlier normalization) explicitly setsdefaultFallback: nullin the input config JSON, this renders asdefaultFallback: nullin the generated YAML, whichagent_config.py's effective resolution logic may not treat as "disabled" the same wayfalseis. Consider coercingNonetoFalsehere for parity with the documented sentinel set (claude | codex | false).🤖 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 `@source/src/story_automator/commands/state.py` around lines 80 - 93, The current assembly of lines uses agent_config.get('defaultFallback', agent_config.get('fallback', False)) which leaves explicit null (None) values through; update the logic that computes the defaultFallback value (located where agent_config, per_task and lines are handled) to coerce None to False before JSON-encoding — e.g., read fallback = agent_config.get('defaultFallback', agent_config.get('fallback', False)); if fallback is None: fallback = False; then use json.dumps(fallback) in the f-string — this ensures explicit null becomes false to match agent_config.py's expected sentinel handling.
🤖 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 `@payload/.claude/skills/bmad-story-automator/data/tmux-commands.md`:
- Around line 76-77: Revise the unclear line "Script: resolve the installed
helper under the active installed skill root." to explicitly state what
<installed-skill-root> means and give concrete examples per provider (e.g., for
Claude use ".claude/skills", for Codex use ".codex/skills"), and/or explain how
to determine it dynamically (e.g., describe checking the user's config or
environment variable that points to the active skill directory); update the
tmux-commands.md entry so readers know exactly which path to use or how to find
it rather than an ambiguous description.
In `@source/src/story_automator/commands/orchestrator_epic_agents.py`:
- Around line 243-254: The explicit-return paths in _resolve_primary_agent and
_resolve_fallback_agent currently return raw values un-normalized, causing
mismatches against downstream lowercase agent IDs; update both functions so the
non-runtime/false branches return the normalized, stripped, lowercase agent id
(e.g. use value = str(raw or "").strip().lower() and return value) or call the
shared resolver used elsewhere instead of returning str(raw), and ensure
_resolve_fallback_agent still maps boolean/false-like inputs to "false" while
normalizing explicit agent names to lowercase.
In `@source/src/story_automator/core/agent_config.py`:
- Around line 110-121: The resolvers return the raw string for non-auto values
which can preserve casing/whitespace and misroute agent selection; update
_resolve_primary_agent to return the already-normalized local variable (value)
instead of str(raw) and update _resolve_fallback_agent to return a normalized
string (e.g., str(value).strip().lower()) so both functions consistently return
stripped lowercase agent names; reference _resolve_primary_agent,
_resolve_fallback_agent, runtime_provider(), and normalize_fallback_value() when
making the change.
In `@source/src/story_automator/core/runtime_layout.py`:
- Around line 172-186: The current logic uses skills_root returned by
resolve_skills_root(STORY_SKILL_NAME) even when that was the synthetic fallback
(root/.claude/skills); change it to only prefer the skill-root-derived marker
when that skills_root actually exists/contains the story skill or was explicitly
selected: in runtime_layout.py, before using skills_root.relative_to(root) and
returning (skills_root.parent / ACTIVE_MARKER_NAME), add a guard that
skills_root.exists() (or another concrete existence check for the installed
story skill inside skills_root) and that the skill is not just the fallback
path; if the check fails, skip to the provider-based branches
(runtime_provider(root) == "codex" -> root/.agents/.story-automator-active, else
root/.claude/.story-automator-active).
---
Outside diff comments:
In `@source/src/story_automator/commands/state.py`:
- Around line 240-251: The validator currently calls required("aiCommand") and
flags empty strings even though aiCommand is deprecated; update the validation
in the state validation function (the block that calls required(...) and
load_policy_for_state) to make aiCommand optional when agentConfig is present:
replace the unconditional required("aiCommand") with a conditional check that
only requires a non-empty aiCommand if state.get("agentConfig") is falsy (or
drop the requirement entirely if you prefer); ensure any existing error messages
still use the required(...) helper for consistency when the check is enforced.
In `@source/src/story_automator/commands/tmux.py`:
- Around line 124-135: The code currently calls agent_type() and then later may
override it with "--agent", causing provider inference to run before
get_project_root(); change the flow so agent is initialized to the raw marker
(e.g., "auto" or the raw env value) instead of calling agent_type() up front,
parse args to possibly set agent string, then compute root = get_project_root()
and only then, if agent == "auto" (or equals the raw env placeholder), call
runtime_provider(root) to get the concrete provider; apply the same change to
the second occurrence around the block referenced at lines 174-178 (replace
upfront agent_type() calls with the raw marker and resolve via
runtime_provider(root) after root is known).
---
Nitpick comments:
In `@payload/.claude/skills/bmad-story-automator/data/tmux-commands.md`:
- Line 80: Replace the inconsistent placeholder in the tmux command assignment
(the script variable name "script") by using {installed-skill-root} instead of
<installed-skill-root>, update the line to match the document's curly-brace
convention (e.g., "{project_root}/
{installed-skill-root}/bmad-story-automator/..."), and add a short note near
that line describing allowable values (for example: use ".claude/skills" for
Claude, ".codex/skills" for Codex) plus an optional shell example hint for users
to derive it dynamically (e.g., show how to detect the skill root via an
environment variable or a simple shell substitution).
In `@source/src/story_automator/commands/state.py`:
- Around line 80-93: The current assembly of lines uses
agent_config.get('defaultFallback', agent_config.get('fallback', False)) which
leaves explicit null (None) values through; update the logic that computes the
defaultFallback value (located where agent_config, per_task and lines are
handled) to coerce None to False before JSON-encoding — e.g., read fallback =
agent_config.get('defaultFallback', agent_config.get('fallback', False)); if
fallback is None: fallback = False; then use json.dumps(fallback) in the
f-string — this ensures explicit null becomes false to match agent_config.py's
expected sentinel handling.
🪄 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: 40da1dbc-d90a-4eb0-b96c-852fac4e119a
📒 Files selected for processing (27)
README.mddocs/cli-reference.mddocs/how-it-works.mddocs/installation-and-layout.mddocs/troubleshooting.mdinstall.shpayload/.claude/skills/bmad-story-automator-review/instructions.xmlpayload/.claude/skills/bmad-story-automator/data/agent-fallback-troubleshooting.mdpayload/.claude/skills/bmad-story-automator/data/agent-fallback.mdpayload/.claude/skills/bmad-story-automator/data/code-review-loop.mdpayload/.claude/skills/bmad-story-automator/data/orchestration-policy.jsonpayload/.claude/skills/bmad-story-automator/data/preflight-prompts.mdpayload/.claude/skills/bmad-story-automator/data/retrospective-automation.mdpayload/.claude/skills/bmad-story-automator/data/retrospective-prompts.mdpayload/.claude/skills/bmad-story-automator/data/stop-hook-config.mdpayload/.claude/skills/bmad-story-automator/data/stop-hook-troubleshooting.mdpayload/.claude/skills/bmad-story-automator/data/tmux-commands.mdpayload/.claude/skills/bmad-story-automator/data/workflow-commands.mdpayload/.claude/skills/bmad-story-automator/templates/state-document.mdscripts/smoke-test.shsource/src/story_automator/commands/orchestrator_epic_agents.pysource/src/story_automator/commands/state.pysource/src/story_automator/commands/tmux.pysource/src/story_automator/core/agent_config.pysource/src/story_automator/core/runtime_layout.pysource/src/story_automator/core/tmux_runtime.pysource/tests/test_runtime_layout.py
✅ Files skipped from review due to trivial changes (8)
- payload/.claude/skills/bmad-story-automator/data/code-review-loop.md
- docs/cli-reference.md
- payload/.claude/skills/bmad-story-automator/data/agent-fallback-troubleshooting.md
- docs/how-it-works.md
- payload/.claude/skills/bmad-story-automator/data/stop-hook-config.md
- docs/installation-and-layout.md
- payload/.claude/skills/bmad-story-automator/data/stop-hook-troubleshooting.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/troubleshooting.md
- payload/.claude/skills/bmad-story-automator/data/retrospective-automation.md
- source/tests/test_runtime_layout.py
|
Review follow-up summary:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
docs/installation-and-layout.md (3)
22-33: ⚡ Quick winClarify when to use
.agents/skillsvs provider-specific roots.The documentation lists three supported roots but doesn't explain when operators should use the generic
.agents/skillsversus provider-specific roots like.codex/skills. Consider adding a brief note about:
- Is
.agents/skillsa recommended generic location for multi-provider projects?- When should operators prefer provider-specific roots?
- Does the runtime auto-detect across all three locations?
This would help operators make informed layout decisions during setup.
🤖 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/installation-and-layout.md` around lines 22 - 33, Add a short clarifying paragraph to the installation-and-layout doc explaining that .agents/skills is the recommended generic root for multi-provider or provider-agnostic projects, provider-specific roots like .codex/skills or .claude/skills should be used when you need provider-specific config or isolation, and that the runtime auto-detects and merges across all supported roots (".agents/skills", ".claude/skills", ".codex/skills") so the installer will update every qualifying root; also mention that payload/.claude/skills is only an internal packaged path and not part of runtime hierarchy.
22-75: ⚡ Quick winConsider defining skill root terminology upfront.
The document uses several related terms throughout:
- "supported skill root" / "supported dependency skill root" (lines 22, 29, 53)
- "qualifying root" / "qualifying skill root" (lines 31, 75)
- "dependency skill root" (line 14)
- "installed skill root" (line 39)
- "target root" (line 33)
While context makes the meaning clear, explicitly defining the relationship between these terms early in the document would help operators understand the install logic:
- Supported roots: The three recognized locations (
.agents/skills,.claude/skills,.codex/skills)- Qualifying roots: Supported roots that contain all required dependency
SKILL.mdfiles- Installed skill root: The qualifying root where
bmad-story-automatorgets installedThis is a low-priority documentation polish item.
🤖 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/installation-and-layout.md` around lines 22 - 75, Add a short "Terminology" section near the top that defines the terms used in the doc: list "Supported roots" and enumerate `.agents/skills`, `.claude/skills`, `.codex/skills`; define "Qualifying roots" as supported roots that contain all required dependency SKILL.md files; define "Installed skill root" / "Target root" as the qualifying root(s) where bmad-story-automator is copied; and clarify "Installed skill payload" as the internal payload path under payload/.claude/skills. Then update usages of the phrases across the file to match these canonical names (e.g., replace mixed uses of "supported dependency skill root", "dependency skill root", "qualifying root", "installed skill root", and "target root" with the chosen terms) so terminology is consistent.
102-103: 💤 Low valueConsider clarifying "still uses" phrasing.
The phrase "Codex still uses the active installed skill root" might be clearer as "Codex uses the active installed skill root for the helper entrypoint, but writes hook/config state..."
The word "still" suggests a contrast with previous behavior, but the contrast here is between skill location and config location, not temporal change.
🤖 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/installation-and-layout.md` around lines 102 - 103, Update the sentence to remove ambiguous "still uses" phrasing: replace "Codex still uses the active installed skill root for this helper, but writes hook/config state to `.codex/hooks.json` and `.codex/config.toml`." with wording like "Codex uses the active installed skill root for the helper entrypoint, but writes hook/config state to `.codex/hooks.json` and `.codex/config.toml`." to clarify the contrast is between skill location and config location; target the sentence containing "Codex still uses the active installed skill root" in docs/installation-and-layout.md.source/tests/test_runtime_layout.py (1)
191-197: ⚡ Quick win
_install_bundlewill fail with an opaqueFileNotFoundErrorif the payload is absent
shutil.copytreeraisesFileNotFoundErrordirectly ifpayload/.claude/skills/bmad-story-automatororbmad-story-automator-reviewdoesn't exist. Because every test that exercises provider-detection or marker logic depends on this helper, the failure surfaces as a cryptic OS error with no hint about the missing payload, rather than a skipped or clearly-labelled test failure.A minimal guard keeps the intent clear and prevents confusion in stripped-down checkouts or isolated package installs:
🛠️ Proposed fix
def _install_bundle(self, runtime_dir: str) -> None: source_skill = REPO_ROOT / "payload" / ".claude" / "skills" / "bmad-story-automator" source_review = REPO_ROOT / "payload" / ".claude" / "skills" / "bmad-story-automator-review" + if not source_skill.is_dir() or not source_review.is_dir(): + self.skipTest(f"payload skills not found under {REPO_ROOT / 'payload'}") target_root = self.project_root / runtime_dir / "skills" target_root.mkdir(parents=True, exist_ok=True) shutil.copytree(source_skill, target_root / "bmad-story-automator") shutil.copytree(source_review, target_root / "bmad-story-automator-review")🤖 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 `@source/tests/test_runtime_layout.py` around lines 191 - 197, The helper _install_bundle currently calls shutil.copytree on source_skill and source_review and will raise an opaque FileNotFoundError if the payload is missing; update _install_bundle to check existence of REPO_ROOT / "payload" / ".claude" / "skills" for both source_skill and source_review before calling shutil.copytree and raise a clearer, descriptive error (or call pytest.skip) that mentions the missing payload path and test dependency (reference symbols: _install_bundle, source_skill, source_review, shutil.copytree, REPO_ROOT, project_root) so failures in test_runtime_layout are explicit and informative.
🤖 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 `@source/src/story_automator/commands/state.py`:
- Around line 88-95: The emitted defaultPrimary can become JSON null if the key
exists with null; mirror the defaultFallback guard by computing a
default_primary variable: retrieve agent_config.get('defaultPrimary') into
default_primary, and if it is None set it to agent_config.get('primary', 'auto')
(or 'auto' if that is missing); then use default_primary in the f-string that
emits the "defaultPrimary" line so you never serialize a Python None to "null".
In `@source/src/story_automator/commands/tmux.py`:
- Line 291: cmd_monitor_session currently only treats "auto" specially and
leaves "runtime" as a literal, causing session_status to receive wrong provider
flags; replace the ad-hoc check in cmd_monitor_session (where agent is read and
tested with if agent == "auto") with the same helper used elsewhere: call
_resolve_agent_selection(...) to normalize the agent and derive the
codex/runtime/provider flags (same approach used by _spawn and _build_cmd) and
then pass the normalized values into session_status; ensure the agent variable
used in session_status calls is the resolved value returned by
_resolve_agent_selection so "runtime" is handled correctly.
---
Nitpick comments:
In `@docs/installation-and-layout.md`:
- Around line 22-33: Add a short clarifying paragraph to the
installation-and-layout doc explaining that .agents/skills is the recommended
generic root for multi-provider or provider-agnostic projects, provider-specific
roots like .codex/skills or .claude/skills should be used when you need
provider-specific config or isolation, and that the runtime auto-detects and
merges across all supported roots (".agents/skills", ".claude/skills",
".codex/skills") so the installer will update every qualifying root; also
mention that payload/.claude/skills is only an internal packaged path and not
part of runtime hierarchy.
- Around line 22-75: Add a short "Terminology" section near the top that defines
the terms used in the doc: list "Supported roots" and enumerate
`.agents/skills`, `.claude/skills`, `.codex/skills`; define "Qualifying roots"
as supported roots that contain all required dependency SKILL.md files; define
"Installed skill root" / "Target root" as the qualifying root(s) where
bmad-story-automator is copied; and clarify "Installed skill payload" as the
internal payload path under payload/.claude/skills. Then update usages of the
phrases across the file to match these canonical names (e.g., replace mixed uses
of "supported dependency skill root", "dependency skill root", "qualifying
root", "installed skill root", and "target root" with the chosen terms) so
terminology is consistent.
- Around line 102-103: Update the sentence to remove ambiguous "still uses"
phrasing: replace "Codex still uses the active installed skill root for this
helper, but writes hook/config state to `.codex/hooks.json` and
`.codex/config.toml`." with wording like "Codex uses the active installed skill
root for the helper entrypoint, but writes hook/config state to
`.codex/hooks.json` and `.codex/config.toml`." to clarify the contrast is
between skill location and config location; target the sentence containing
"Codex still uses the active installed skill root" in
docs/installation-and-layout.md.
In `@source/tests/test_runtime_layout.py`:
- Around line 191-197: The helper _install_bundle currently calls
shutil.copytree on source_skill and source_review and will raise an opaque
FileNotFoundError if the payload is missing; update _install_bundle to check
existence of REPO_ROOT / "payload" / ".claude" / "skills" for both source_skill
and source_review before calling shutil.copytree and raise a clearer,
descriptive error (or call pytest.skip) that mentions the missing payload path
and test dependency (reference symbols: _install_bundle, source_skill,
source_review, shutil.copytree, REPO_ROOT, project_root) so failures in
test_runtime_layout are explicit and informative.
🪄 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: b9b8c605-a193-445f-ac4f-87b50d56123c
📒 Files selected for processing (14)
README.mddocs/installation-and-layout.mdinstall.shpayload/.claude/skills/bmad-story-automator/data/orchestration-policy.jsonpayload/.claude/skills/bmad-story-automator/data/tmux-commands.mdscripts/smoke-test.shsource/src/story_automator/commands/orchestrator_epic_agents.pysource/src/story_automator/commands/state.pysource/src/story_automator/commands/tmux.pysource/src/story_automator/core/agent_config.pysource/src/story_automator/core/runtime_layout.pysource/src/story_automator/core/runtime_policy.pysource/tests/test_runtime_layout.pysource/tests/test_state_policy_metadata.py
🚧 Files skipped from review as they are similar to previous changes (9)
- payload/.claude/skills/bmad-story-automator/data/orchestration-policy.json
- payload/.claude/skills/bmad-story-automator/data/tmux-commands.md
- source/src/story_automator/core/agent_config.py
- install.sh
- scripts/smoke-test.sh
- source/src/story_automator/commands/orchestrator_epic_agents.py
- README.md
- source/tests/test_state_policy_metadata.py
- source/src/story_automator/core/runtime_layout.py
dickymoore
force-pushed
the
dicky/codex-runtime-support
branch
from
0217624 to
cf96221
Compare
bma-d
left a comment
bma-d
left a comment
There was a problem hiding this comment.
Review pass complete. P0 clean, P1 not clean. I focused on preserving existing Claude behavior and keeping runtime/provider concerns separated from child-agent selection. Two items could not be placed inline because the affected files/lines are not in this PR diff: docs/state-and-resume.md still hard-codes .claude/.story-automator-active, and steps-v/step-v-01-check.md still treats deprecated aiCommand as the only valid runtime command field even though runtime validation accepts agentConfig. Those should still be fixed before merge.
The main architectural ask: split runtime context from child-agent selection. Provider/layout, skill root, marker root, hook target, and child agent should be separate values; AI_AGENT should not decide hook/config layout.
| # Provider controls hook/config syntax: Claude writes settings.json, Codex writes | ||
| # hooks.json plus config.toml. Marker paths are resolved separately so they can | ||
| # follow the installed skill root in mixed or migrated workspaces. | ||
| for name in ("BMAD_RUNTIME_PROVIDER", "STORY_AUTOMATOR_RUNTIME_PROVIDER", "AI_AGENT"): |
There was a problem hiding this comment.
P1: AI_AGENT is a child-agent selector, but runtime_provider() also treats it as the top-level hook/config provider. That means a Claude-installed project can export AI_AGENT=codex for fallback/worker selection and then ensure-stop-hook writes .codex/hooks.json instead of .claude/settings.json, losing the existing Claude stop-hook safeguard.
Suggested fix: remove AI_AGENT from provider/layout detection. Use only explicit provider env such as BMAD_RUNTIME_PROVIDER / STORY_AUTOMATOR_RUNTIME_PROVIDER, installed skill root/session metadata, or a dedicated runtime context object for hook/config layout. Keep AI_AGENT scoped to spawned child sessions. Add a regression for .claude/skills/bmad-story-automator plus AI_AGENT=codex still writing .claude/settings.json.
There was a problem hiding this comment.
Addressed in b3a4c9e: runtime_provider() no longer reads AI_AGENT, so child-agent selection no longer changes top-level hook/config layout. Added regression coverage for a Claude-installed project with AI_AGENT=codex still resolving Claude marker/provider behavior and still writing Claude hook config.
| } | ||
|
|
||
|
|
||
| def ensure_codex_stop_hook(*, project_root: Path, command: str, timeout: int) -> dict[str, Any]: |
There was a problem hiding this comment.
P1: Codex hook installation writes .codex/config.toml and .codex/hooks.json, then later reports Codex Stop hook verified. without checking whether Codex actually trusts/runs the unmanaged hook. Current Codex hook state exposes trust/hash state, so a configured-but-untrusted hook can look verified here while the stop safeguard is inactive.
Suggested fix: after writing Codex hook config, query/inspect Codex hook state for the project and match the Story Automator Stop hook. If the hook is unmanaged and not trusted, return a pending-trust state instead of verified, with an explicit approval instruction. If you add an approval path, make it a separate user-approved trust step, not an automatic write hidden inside install.
There was a problem hiding this comment.
Addressed in b3a4c9e: Codex stop-hook setup now distinguishes configured vs pending-trust vs verified state instead of always implying the safeguard is active. ensure_codex_stop_hook() now returns trust-aware status fields/messages, and coverage locks the configured-but-untrusted path.
| config.default_primary = data.get("defaultPrimary") or data.get("primary") or "claude" | ||
| config.default_fallback = data.get("defaultFallback") or data.get("fallback") or "codex" | ||
| config.default_primary = data.get("defaultPrimary") or data.get("primary") or "auto" | ||
| config.default_fallback = data.get("defaultFallback") or data.get("fallback") or "false" |
There was a problem hiding this comment.
P2: This changes the missing/legacy fallback default from Codex to disabled fallback. On automator/main, empty config and {defaultPrimary:"claude"} resolved to Claude primary plus Codex fallback; this PR resolves both to no fallback. That breaks existing Claude-first retry behavior unless every existing state/config explicitly opts back in.
Suggested fix: preserve the legacy fallback default for missing/unversioned config, or introduce an explicit config version/migration where only new-version configs default defaultFallback to false. Add tests for {} and {"defaultPrimary":"claude"} so the compatibility choice is locked.
There was a problem hiding this comment.
Addressed in b3a4c9e: missing/unversioned fallback config now preserves the legacy Codex fallback instead of silently disabling fallback. Added targeted regressions for {} and {"defaultPrimary":"claude"} plus explicit-disabled fallback coverage.
| - `.codex/skills` | ||
|
|
||
| Unlike the older workflow-root layout, this Python port installs into the pure skill tree. | ||
| Use `.agents/skills` for provider-agnostic or multi-provider projects. Use `.claude/skills` or `.codex/skills` when provider-specific isolation is useful. For Codex, installed skill assets live under a supported root such as `.agents/skills` or `.codex/skills`, while stop-hook and runtime config files are managed under `.codex/`. |
There was a problem hiding this comment.
P2: The docs describe .agents/skills as provider-agnostic/multi-provider, but runtime inference treats .agents as Codex. That separation matters: installing into a supposedly neutral root can change hook/config semantics to Codex.
Suggested fix: either make .agents truly provider-neutral and require explicit runtime provider context for hook/config writes, or document .agents as Codex-owned. I prefer the former so Codex support does not alter Claude behavior in mixed installs.
There was a problem hiding this comment.
Partially addressed in b3a4c9e: this pass documents the corrected provider resolution model and explicitly separates provider/layout detection from AI_AGENT. I did not redesign .agents to be provider-neutral in this PR; that broader model change is intentionally deferred to a follow-up if we decide to pursue it.
There was a problem hiding this comment.
Updated in 05dad8c: the installation/layout doc now distinguishes install-root behavior from runtime provider/layout behavior instead of describing .agents/skills as simply provider-agnostic. It now states the current runtime fallback model explicitly: explicit provider env wins first, active skill-root context comes next, and .agents / .codex fallback to Codex-style hook/config layout while .claude falls back to Claude-style layout.
| Unlike the older workflow-root layout, this Python port installs into the pure skill tree. | ||
| Use `.agents/skills` for provider-agnostic or multi-provider projects. Use `.claude/skills` or `.codex/skills` when provider-specific isolation is useful. For Codex, installed skill assets live under a supported root such as `.agents/skills` or `.codex/skills`, while stop-hook and runtime config files are managed under `.codex/`. | ||
|
|
||
| There is no runtime precedence among these roots. Claude-only, Codex-only, and mixed projects are supported. If more than one root contains all required dependency `SKILL.md` files, the installer updates each qualifying root. If a root is present but missing required skill entrypoints while another root qualifies, the incomplete root is left unchanged. |
There was a problem hiding this comment.
P2: This says there is no runtime precedence among roots, but the resolver has a real order: explicit BMAD_SKILLS_ROOT, current installed helper root, project .agents, project .claude, project .codex, then home roots. Operators need the documented rule to predict marker path, asset resolution, and contract resolution in mixed installs.
Suggested fix: document the actual precedence, or make mixed-root execution fail/ask for an explicit root/provider. Add a mixed-root test that enforces whichever rule you document.
There was a problem hiding this comment.
Partially addressed in b3a4c9e: the doc now records the provider-resolution order used by the merge-blocker fix and makes AI_AGENT non-authoritative for provider/layout selection. I did not expand this into a broader mixed-root execution redesign in this PR; any deeper precedence/model cleanup stays in the follow-up bucket.
There was a problem hiding this comment.
Updated in 05dad8c: the doc no longer says there is no runtime precedence. It now separates installer behavior from runtime behavior and documents the active runtime resolution order (BMAD_SKILLS_ROOT, current helper root, project .agents, .claude, .codex, then home roots).
| self._install_bundle(".agents") | ||
|
|
||
| self.assertEqual(runtime_provider(str(self.project_root)), "codex") | ||
| self.assertEqual(active_marker_path(self.project_root), self.project_root / ".agents" / ".story-automator-active") |
There was a problem hiding this comment.
P2: These assertions compare resolved runtime paths against unresolved TemporaryDirectory() paths. On macOS, /var resolves through /private/var, so npm run test:python fails locally with 7 runtime-layout path mismatches even though the behavior under test is otherwise the same.
Suggested fix: compare resolved expected paths, e.g. (self.project_root / ".agents" / ".story-automator-active").resolve(), or normalize both sides in a small helper. Keep separate string-entry tests for unresolved project-relative display values like .agents/.story-automator-active.
There was a problem hiding this comment.
Leaving this one deferred on purpose. The current merge-blocker pass stayed focused on runtime/provider correctness and stop-hook safety; macOS temp-path normalization is still planned as a tightly-scoped follow-up test-robustness PR.
There was a problem hiding this comment.
Folded back into the main PR in 05dad8c: the runtime-layout tests now compare normalized/resolved path targets so the targeted Python test subset is portable across macOS /var vs /private/var path layouts as well as Linux.
|
|
||
| ```bash | ||
| scripts=".claude/skills/bmad-story-automator/scripts/story-automator" | ||
| scripts="<installed-skill-root>/bmad-story-automator/scripts/story-automator" |
There was a problem hiding this comment.
P3: This is now a copy-paste command, but <installed-skill-root> is a literal placeholder and will fail if operators paste it during setup or incident debugging.
Suggested fix: include a tiny root-discovery snippet over .agents/skills, .claude/skills, and .codex/skills, then set scripts from the discovered executable. Mirror the same pattern in troubleshooting examples.
There was a problem hiding this comment.
Deferring this intentionally to the docs/operator-ergonomics follow-up PR. I kept the blocker pass limited to runtime/provider correctness and only updated the contract-critical docs needed for those behavior changes.
There was a problem hiding this comment.
Folded back into the main PR in 05dad8c: the CLI reference is now copy-paste-safe. It resolves the installed helper from supported skill roots first and then uses $scripts in the examples instead of the literal <installed-skill-root> placeholder.
| .claude/skills/bmad-story-automator/scripts/story-automator tmux-wrapper list --project-only | ||
| .claude/skills/bmad-story-automator/scripts/story-automator orchestrator-helper state-list _bmad-output/story-automator | ||
| .claude/skills/bmad-story-automator/scripts/story-automator orchestrator-helper verify-code-review 1.2 | ||
| <installed-skill-root>/bmad-story-automator/scripts/story-automator tmux-wrapper list --project-only |
There was a problem hiding this comment.
P3: Same placeholder issue as the CLI reference: these incident-response checks are likely to be copy-pasted, and <installed-skill-root> is not executable.
Suggested fix: define scripts once using documented root discovery, then use $scripts tmux-wrapper list --project-only, $scripts orchestrator-helper state-list ..., and $scripts orchestrator-helper verify-code-review ....
There was a problem hiding this comment.
Deferring this intentionally to the docs/operator-ergonomics follow-up PR alongside the CLI reference placeholder cleanup so both operator-facing command examples can be fixed together.
There was a problem hiding this comment.
Folded back into the main PR in 05dad8c: the troubleshooting guide now uses the same supported-root discovery snippet and $scripts examples instead of the literal <installed-skill-root> placeholder.
| if os.environ.get("STORY_AUTOMATOR_CHILD", "").lower() == "true": | ||
| return 0 | ||
| marker = Path(os.getcwd()) / ".claude" / ".story-automator-active" | ||
| marker = active_marker_path(Path(os.getcwd())) |
There was a problem hiding this comment.
P2: The stop hook discards the hook payload and resolves the marker from os.getcwd(). The Codex hook config stores only the command, with no project-root env. If Codex invokes the hook from a subdirectory, this checks <subdir>/.agents|.codex/.story-automator-active instead of the project-root marker and can silently allow stopping during an active orchestration.
Suggested fix: pass/export the project root in the installed hook command, or parse the hook stdin payload for cwd/session context and normalize to the project root deterministically before calling active_marker_path(). Add a test where the marker is at project root but the hook process cwd is a nested directory.
There was a problem hiding this comment.
Addressed in b3a4c9e: installed stop-hook commands now export PROJECT_ROOT, and the hook resolves the active marker from that project root instead of ambient cwd. Added regression coverage for nested-directory invocation with a project-root marker.
| agent = agent or _raw_agent_selection() | ||
| story_prefix = story_id.replace(".", "-") | ||
| root = get_project_root() | ||
| agent = _resolve_agent_selection(agent, root) |
There was a problem hiding this comment.
P2: Legacy AI_COMMAND is honored after auto has already resolved through runtime_provider(). In an .agents/Codex install with AI_COMMAND="claude ..." and no AI_AGENT, build-cmd emits a Claude command while spawn/monitor classify the session as Codex. That skips Claude-specific completion handling and can leave a finished Claude child stuck or timed out.
Suggested fix: when AI_COMMAND is present without AI_AGENT, infer the agent from the command before resolving/spawning, or force the whole legacy path to Claude consistently. Add a regression that AI_COMMAND=claude ... records/monitors the session as Claude even in a Codex-layout install.
There was a problem hiding this comment.
Addressed in b3a4c9e: when legacy AI_COMMAND is present without AI_AGENT, build/monitor behavior now infers the agent from the command instead of classifying the session through the Codex layout by default. Added regression coverage for a Claude legacy command path.
|
Merge-blocker rework is now pushed on Addressed in this pass:
Validation run for this pass:
Intentionally deferred to tightly-scoped follow-up PRs:
|
2 participants
Summary
Adds Codex runtime support alongside Claude Code in BMad Automator, rebased onto the upstream self-contained
skills/plugin layout.This PR now folds in the correctness, compatibility, platform-testability, and operator-facing doc fixes needed to make the runtime change set robust instead of leaving those concerns split across too many follow-ups.
Main fixes in this PR
AI_AGENTto decide hook/config layout and marker defaultsPROJECT_ROOTthrough installed stop-hook commands so marker lookup does not depend on the caller's working directoryAI_COMMANDclassification consistent with build/monitor behavioraiCommand/agentConfigruntime model already supported by the codeScope Notes
This PR is still intentionally bounded to Codex runtime support plus the fixes needed to make that behavior safe, testable, and reviewable.
What remains intentionally out of scope:
.agentsneutrality beyond documenting the current runtime model accuratelyTests
PYTHONPATH=skills/bmad-story-automator/src python3 -m pytest tests/test_runtime_layout.py tests/test_stop_hooks.py tests/test_state_policy_metadata.py tests/test_success_verifiers.py -q126 passedCompanion Work
Remaining Follow-ups
.agentsruntime semantics, if we decide to go beyond documenting the current behavior.