feat: add inspectable workflow snapshots - #2601
Conversation
alexeyv
changed the title
e6c1adb to
7f7cc46
Compare
|
@coderabbitai review |
Greptile SummaryThis PR introduces an immutable, hash-addressed snapshot renderer for the Dev Auto skill. Instead of the agent interpreting mutable skill sources directly, a single
Confidence Score: 5/5Safe to merge; the atomic publish design handles concurrent renders and corruption correctly across all changed paths. The renderer publish path uses atomic os.rename, verifies existing generations byte-for-byte, and routes every error through a clean HALT message rather than a traceback. Config and customization layers are strictly validated. Installer exclusions are additive and covered by the new test suite. The ui.js fix is confirmed by the partial-config assertion in suite 49. No logic paths were found that could silently produce wrong output or corrupt an existing snapshot. Files Needing Attention: No files require special attention beyond the minor test-cleanup note on test/test-dev-auto-renderer.js.
|
| Filename | Overview |
|---|---|
| src/scripts/render_skill.py | New renderer: loads config/customization, resolves tokens, and publishes immutable hash-addressed snapshot directories. Correctly handles concurrent renders via atomic os.rename, verifies existing generations byte-for-byte, and gates renderer identity into the generation hash. PEP 723 script metadata is present. |
| src/scripts/config_utils.py | New shared module for strict TOML loading and structural merge. Validates keyed-array identifiers as non-empty strings, raises ConfigError on any malformed layer, and is imported only so it correctly inherits sys.dont_write_bytecode from its callers. |
| src/scripts/resolve_config.py | Refactored to delegate merge logic to config_utils. Adds PEP 723 script metadata, sys.dont_write_bytecode, and a clean ModuleNotFoundError guard that emits a human-readable version message on Python less than 3.11. |
| src/scripts/resolve_customization.py | Same refactor pattern as resolve_config.py. Adds an explicit --project-root flag so callers can bypass the heuristic find_project_root walk when the project root is already known. |
| test/test-dev-auto-renderer.js | 21 black-box tests covering immutability, concurrency, hash identity, token resolution, error halts, and path-safety. The initial fixture() call at the top of main() sits outside a test() wrapper and outside a finally block, so a partial fixture failure would skip temp-dir cleanup. |
| test/test-installation-components.js | New Test Suite 49 exercises the full installer-to-renderer pipeline, confirming render_skill.py and config_utils.py reach _bmad/scripts, that the render .gitignore is seeded, that render snapshots are excluded from custom-file detection, and that an end-to-end uv run dispatch succeeds. |
| tools/installer/core/installer.js | Adds render dir to nonModuleDirs at both scan callsites, excludes render/ from custom-file detection, and seeds _bmad/render/.gitignore during _installSharedScripts. |
| tools/installer/ui.js | Bug fix: reads existingConfig.core from disk instead of collectedConfig.core in both the partial-CLI and --yes branches; adds a defaultConfig layer so omitted values get defaults when --yes is combined with partial CLI options. |
| src/bmm-skills/4-implementation/bmad-dev-auto/SKILL.md | Stripped to a minimal entry point: run uv run --no-cache render_skill.py, then follow the dispatched workflow.md. All workflow logic moved to the rendered snapshot. |
| src/bmm-skills/4-implementation/bmad-dev-auto/workflow.md | New source file rendered into every snapshot. Uses compile-time config tokens, customization tokens, and snapshot cross-reference tokens — all resolved to absolute paths at render time. |
Sequence Diagram
sequenceDiagram
participant A as Agent
participant S as SKILL.md
participant R as render_skill.py
participant C as config_utils.py
participant FS as Snapshot dir
A->>S: read SKILL.md
S-->>A: uv run --no-cache render_skill.py ...
A->>R: uv run (PEP 723 provisions Python 3.11+)
R->>C: load_central_config(project_root)
C-->>R: merged TOML (4 layers)
R->>C: load_customization(project_root, skill_dir)
C-->>R: merged customization (3 layers)
R->>R: load_sources - rglob .md skip SKILL.md
R->>R: resolve_replacements - config shorthand workflow tokens
R->>R: "hash identity = gen_hash"
alt destination already exists
R->>FS: verify_existing - compare manifest + rehash outputs
FS-->>R: valid or RenderError
else first render or concurrent loser
R->>FS: atomic os.rename staging to destination
Note over R,FS: concurrent loser verifies on OSError
end
R-->>A: read and follow /abs/path/workflow.md
A->>FS: read dispatched workflow.md
Reviews (2): Last reviewed commit: "feat: add inspectable workflow snapshots" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -959,7 +967,7 @@ class Installer { | |||
|
|
|||
There was a problem hiding this comment.
nonModuleDirs exclusion for render is duplicated across three callsites
The same 'render' exclusion now lives in installer.js (two separate nonModuleDirs Set literals, lines 967 and 1064) and again in official-modules.js line 922. All three are independent local declarations, so adding a future non-module directory means updating three separate places. A shared constant defined once and imported by all three would keep them in sync automatically.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/installer/core/installer.js
Line: 967
Comment:
**`nonModuleDirs` exclusion for `render` is duplicated across three callsites**
The same `'render'` exclusion now lives in `installer.js` (two separate `nonModuleDirs` `Set` literals, lines 967 and 1064) and again in `official-modules.js` line 922. All three are independent local declarations, so adding a future non-module directory means updating three separate places. A shared constant defined once and imported by all three would keep them in sync automatically.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (28)
📝 WalkthroughWalkthroughThe PR adds shared TOML configuration resolution and an immutable skill snapshot renderer, rewrites dev-auto execution around rendered workflow snapshots, updates installer handling for generated render state, and adds renderer, configuration, installation, CI, and documentation coverage. ChangesDev Auto Rendering
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as render_skill.py CLI
participant Renderer as render()
participant Config as config_utils
participant Snapshot as Render snapshot
CLI->>Renderer: Render project root and skill
Renderer->>Config: Load configuration layers
Config-->>Renderer: Return merged values
Renderer->>Snapshot: Publish verified snapshot and manifest
Snapshot-->>CLI: Return absolute workflow.md path
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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: 3
🧹 Nitpick comments (4)
tools/validate-file-refs.js (1)
83-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider collapsing the per-skill
render/entries into a single prefix.Nothing under
render/ever exists insrc/, so'render/'would coverbmad-quick-dev,bmad-dev-auto, and every future snapshot-rendering skill without another allowlist edit each time.♻️ Proposed simplification
-const INSTALL_ONLY_PATHS = ['_config/', 'custom/', 'render/bmad-quick-dev/', 'render/bmad-dev-auto/']; +const INSTALL_ONLY_PATHS = ['_config/', 'custom/', 'render/'];🤖 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 `@tools/validate-file-refs.js` at line 83, Update INSTALL_ONLY_PATHS in validate-file-refs.js to replace the individual render/bmad-quick-dev/ and render/bmad-dev-auto/ entries with the broader render/ prefix, while preserving the existing _config/ and custom/ entries.tools/installer/core/installer.js (2)
689-696: 📐 Maintainability & Code Quality | 🔵 TrivialSnapshots under
_bmad/render/are never pruned.Generations are content-addressed on
renderer_sha256+ resolved values, so every installer upgrade or config change strands the prior generation permanently. Over many upgrades this accumulates in every user project with nothing reclaiming it. Consider a retention pass (keep the current generation per skill+root, drop the rest) during install, or document the directory as safe to delete.🤖 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 `@tools/installer/core/installer.js` around lines 689 - 696, Update the installer flow around the render directory setup and installedFiles tracking to prune obsolete snapshots under _bmad/render after installation. Retain the current generation for each skill and resolved root, remove older content-addressed generations, and preserve the active render files and .gitignore.
970-970: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift"Not a module directory" is hardcoded in four places. Adding
renderrequired an identical edit at each site; there is no shared constant, so the next generated-state directory will silently be treated as a module wherever a site is missed. Export one list (e.g.NON_MODULE_DIRS) from a shared installer module and consume it everywhere.
tools/installer/core/installer.js#L970-L970: replace the inlinenonModuleDirsSet ingenerateModuleConfigswith the shared constant.tools/installer/core/installer.js#L1067-L1067: replace the duplicate inline Set inmergeModuleHelpCatalogswith the same shared constant.tools/installer/modules/official-modules.js#L925-L925: replace the inline Set inloadExistingConfig's legacy fallback with the shared constant.tools/installer/core/manifest-generator.js#L756-L756: replace theentry.name === 'render'literal inscanInstalledModuleswith a lookup against the shared constant.🤖 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 `@tools/installer/core/installer.js` at line 970, Define and export a shared NON_MODULE_DIRS collection from an appropriate installer module, then reuse it in tools/installer/core/installer.js lines 970-970 and 1067-1067, and tools/installer/modules/official-modules.js lines 925-925, replacing each inline Set. In tools/installer/core/manifest-generator.js line 756, update scanInstalledModules to check entry.name against the shared collection instead of the hardcoded render literal.src/scripts/tests/test_config_utils.py (1)
50-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
load_toml(..., required=True)and the non-table branches.The optional-layer paths are covered, but the
required=Truemissing-file error and the "not a file" / "did not parse to a table" branches ofload_tomlhave no test here — those are the branches that fire on a broken_bmad/config.toml.🧪 Suggested additional cases
def test_missing_required_layer_is_rejected(self): with tempfile.TemporaryDirectory() as temp_dir: path = Path(temp_dir) / "config.toml" with self.assertRaisesRegex(ConfigError, "required TOML file not found"): load_toml(path, required=True) def test_directory_layer_is_rejected(self): with tempfile.TemporaryDirectory() as temp_dir: path = Path(temp_dir) / "config.toml" path.mkdir() with self.assertRaisesRegex(ConfigError, "not a file"): load_toml(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 `@src/scripts/tests/test_config_utils.py` around lines 50 - 62, Extend the load_toml tests to cover missing required files and non-table parse results. Add a test asserting load_toml(path, required=True) raises ConfigError with “required TOML file not found,” a directory path raises ConfigError mentioning “not a file,” and a valid TOML document that parses to a non-table value raises the corresponding ConfigError.
🤖 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 `@src/scripts/config_utils.py`:
- Around line 58-77: Update _merge_arrays to track keys independently while
processing base and override, and raise ConfigError when either layer contains a
duplicate keyed entry. Preserve valid cross-layer matching so an override may
replace a base item with the same key, while retaining the existing merged
ordering and append behavior for new keys.
In `@test/test-dev-auto-renderer.js`:
- Around line 87-112: Remove the hard-coded Python 3.11 arguments from both
renderer invocations in test/test-dev-auto-renderer.js, specifically run() and
runAsync(), so uv resolves the interpreter from script metadata; also remove the
same arguments from the Suite 49 spawnSync invocation in
test/test-installation-components.js at lines 3683-3687. Preserve all other
command arguments and test behavior.
In `@test/test-installation-components.js`:
- Around line 3683-3693: Guard the `render49.stdout` access when deriving
`dispatch49`, using an empty-string fallback before calling `.trim()` so
unavailable `uv` still reaches the existing assertion. Keep the
`render49.status` check and `${render49.stdout}${render49.stderr}` diagnostics
unchanged.
---
Nitpick comments:
In `@src/scripts/tests/test_config_utils.py`:
- Around line 50-62: Extend the load_toml tests to cover missing required files
and non-table parse results. Add a test asserting load_toml(path, required=True)
raises ConfigError with “required TOML file not found,” a directory path raises
ConfigError mentioning “not a file,” and a valid TOML document that parses to a
non-table value raises the corresponding ConfigError.
In `@tools/installer/core/installer.js`:
- Around line 689-696: Update the installer flow around the render directory
setup and installedFiles tracking to prune obsolete snapshots under _bmad/render
after installation. Retain the current generation for each skill and resolved
root, remove older content-addressed generations, and preserve the active render
files and .gitignore.
- Line 970: Define and export a shared NON_MODULE_DIRS collection from an
appropriate installer module, then reuse it in tools/installer/core/installer.js
lines 970-970 and 1067-1067, and tools/installer/modules/official-modules.js
lines 925-925, replacing each inline Set. In
tools/installer/core/manifest-generator.js line 756, update scanInstalledModules
to check entry.name against the shared collection instead of the hardcoded
render literal.
In `@tools/validate-file-refs.js`:
- Line 83: Update INSTALL_ONLY_PATHS in validate-file-refs.js to replace the
individual render/bmad-quick-dev/ and render/bmad-dev-auto/ entries with the
broader render/ prefix, while preserving the existing _config/ and custom/
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 429f8cfe-3da7-4ebe-b317-3fbb35e4bba3
📒 Files selected for processing (24)
.github/workflows/publish.yaml.github/workflows/quality.yamldocs/reference/dev-auto.mddocs/zh-cn/reference/dev-auto.mdpackage.jsonsrc/bmm-skills/4-implementation/bmad-dev-auto/SKILL.mdsrc/bmm-skills/4-implementation/bmad-dev-auto/step-01-clarify-and-route.mdsrc/bmm-skills/4-implementation/bmad-dev-auto/step-02-plan.mdsrc/bmm-skills/4-implementation/bmad-dev-auto/step-03-implement.mdsrc/bmm-skills/4-implementation/bmad-dev-auto/step-04-review.mdsrc/bmm-skills/4-implementation/bmad-dev-auto/workflow.mdsrc/scripts/config_utils.pysrc/scripts/render_skill.pysrc/scripts/resolve_config.pysrc/scripts/resolve_customization.pysrc/scripts/tests/test_config_utils.pysrc/scripts/tests/test_resolve_config.pysrc/scripts/tests/test_resolve_customization.pytest/test-dev-auto-renderer.jstest/test-installation-components.jstools/installer/core/installer.jstools/installer/core/manifest-generator.jstools/installer/modules/official-modules.jstools/validate-file-refs.js
| def _merge_arrays(base: list[Any], override: list[Any]) -> list[Any]: | ||
| keyed_field = _detect_keyed_merge_field(base + override) | ||
| if keyed_field is None: | ||
| return list(base) + list(override) | ||
|
|
||
| result: list[Any] = [] | ||
| index_by_key: dict[str, int] = {} | ||
| for item in base: | ||
| copied = dict(item) | ||
| index_by_key[copied[keyed_field]] = len(result) | ||
| result.append(copied) | ||
| for item in override: | ||
| copied = dict(item) | ||
| key = copied[keyed_field] | ||
| if key in index_by_key: | ||
| result[index_by_key[key]] = copied | ||
| else: | ||
| index_by_key[key] = len(result) | ||
| result.append(copied) | ||
| return result |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject duplicate keyed entries within a single layer.
A duplicate id/code in base remains twice in result; a duplicate in override silently keeps only the last entry. This breaks the stated identity-based merge contract and can render conflicting configuration. Track keys separately per layer and raise ConfigError on duplicates; matching a key across layers should remain valid.
Proposed fix
result: list[Any] = []
index_by_key: dict[str, int] = {}
+ base_keys: set[str] = set()
for item in base:
copied = dict(item)
- index_by_key[copied[keyed_field]] = len(result)
+ key = copied[keyed_field]
+ if key in base_keys:
+ raise ConfigError(f"duplicate keyed array identifier `{key}`")
+ base_keys.add(key)
+ index_by_key[key] = len(result)
result.append(copied)
+ override_keys: set[str] = set()
for item in override:
copied = dict(item)
key = copied[keyed_field]
+ if key in override_keys:
+ raise ConfigError(f"duplicate keyed array identifier `{key}`")
+ override_keys.add(key)
if key in index_by_key:📝 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.
| def _merge_arrays(base: list[Any], override: list[Any]) -> list[Any]: | |
| keyed_field = _detect_keyed_merge_field(base + override) | |
| if keyed_field is None: | |
| return list(base) + list(override) | |
| result: list[Any] = [] | |
| index_by_key: dict[str, int] = {} | |
| for item in base: | |
| copied = dict(item) | |
| index_by_key[copied[keyed_field]] = len(result) | |
| result.append(copied) | |
| for item in override: | |
| copied = dict(item) | |
| key = copied[keyed_field] | |
| if key in index_by_key: | |
| result[index_by_key[key]] = copied | |
| else: | |
| index_by_key[key] = len(result) | |
| result.append(copied) | |
| return result | |
| def _merge_arrays(base: list[Any], override: list[Any]) -> list[Any]: | |
| keyed_field = _detect_keyed_merge_field(base + override) | |
| if keyed_field is None: | |
| return list(base) + list(override) | |
| result: list[Any] = [] | |
| index_by_key: dict[str, int] = {} | |
| base_keys: set[str] = set() | |
| for item in base: | |
| copied = dict(item) | |
| key = copied[keyed_field] | |
| if key in base_keys: | |
| raise ConfigError(f"duplicate keyed array identifier `{key}`") | |
| base_keys.add(key) | |
| index_by_key[key] = len(result) | |
| result.append(copied) | |
| override_keys: set[str] = set() | |
| for item in override: | |
| copied = dict(item) | |
| key = copied[keyed_field] | |
| if key in override_keys: | |
| raise ConfigError(f"duplicate keyed array identifier `{key}`") | |
| override_keys.add(key) | |
| if key in index_by_key: | |
| result[index_by_key[key]] = copied | |
| else: | |
| index_by_key[key] = len(result) | |
| result.append(copied) | |
| return result |
🤖 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 `@src/scripts/config_utils.py` around lines 58 - 77, Update _merge_arrays to
track keys independently while processing base and override, and raise
ConfigError when either layer contains a duplicate keyed entry. Preserve valid
cross-layer matching so an override may replace a base item with the same key,
while retaining the existing merged ordering and append behavior for new keys.
| function run(fix, cwd = fix.project) { | ||
| return spawnSync( | ||
| 'uv', | ||
| ['run', '--python', '3.11', path.join(fix.bmad, 'scripts', 'render_skill.py'), '--project-root', fix.project, '--skill', fix.skill], | ||
| { | ||
| cwd, | ||
| encoding: 'utf8', | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| function runAsync(fix) { | ||
| return new Promise((resolve) => { | ||
| const child = spawn( | ||
| 'uv', | ||
| ['run', '--python', '3.11', path.join(fix.bmad, 'scripts', 'render_skill.py'), '--project-root', fix.project, '--skill', fix.skill], | ||
| { cwd: fix.project }, | ||
| ); | ||
| let stdout = ''; | ||
| let stderr = ''; | ||
| child.stdout.on('data', (chunk) => (stdout += chunk)); | ||
| child.stderr.on('data', (chunk) => (stderr += chunk)); | ||
| child.on('error', (error) => resolve({ status: null, stdout, stderr: `${stderr}${error.message}` })); | ||
| child.on('close', (status) => resolve({ status, stdout, stderr })); | ||
| }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Both renderer test suites pin uv run --python 3.11, contradicting the shipped unpinned contract. test/test-installation-components.js line 3658 asserts the installed SKILL.md must not pin --python, so the script-metadata-driven resolution path that real users hit is never executed by any test, and both suites hard-depend on uv being able to provide exactly 3.11.
test/test-dev-auto-renderer.js#L87-L112: drop'--python', '3.11'from the argv in bothrun()andrunAsync()so the renderer resolves its interpreter from script metadata.test/test-installation-components.js#L3683-L3687: drop'--python', '3.11'from the Suite 49spawnSyncargv for the same reason.
📍 Affects 2 files
test/test-dev-auto-renderer.js#L87-L112(this comment)test/test-installation-components.js#L3683-L3687
🤖 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 `@test/test-dev-auto-renderer.js` around lines 87 - 112, Remove the hard-coded
Python 3.11 arguments from both renderer invocations in
test/test-dev-auto-renderer.js, specifically run() and runAsync(), so uv
resolves the interpreter from script metadata; also remove the same arguments
from the Suite 49 spawnSync invocation in test/test-installation-components.js
at lines 3683-3687. Preserve all other command arguments and test behavior.
| const render49 = spawnSync( | ||
| 'uv', | ||
| ['run', '--python', '3.11', path.join(scripts49, 'render_skill.py'), '--project-root', root49, '--skill', skill49], | ||
| { encoding: 'utf8' }, | ||
| ); | ||
| const dispatch49 = render49.stdout.trim().replace(/^read and follow /, ''); | ||
| assert( | ||
| render49.status === 0 && path.isAbsolute(dispatch49) && (await fs.pathExists(dispatch49)), | ||
| 'installer-produced dev-auto tree renders and dispatches end to end', | ||
| `${render49.stdout}${render49.stderr}`, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard render49.stdout before .trim().
When uv is unavailable, spawnSync returns status: null and stdout: null, so line 3688 throws a TypeError that surfaces as an opaque "Test Suite 49 setup failed" instead of the intended assertion with the ${render49.stdout}${render49.stderr} diagnostics.
🛡️ Proposed guard
- const dispatch49 = render49.stdout.trim().replace(/^read and follow /, '');
+ const dispatch49 = (render49.stdout || '').trim().replace(/^read and follow /, '');
assert(
- render49.status === 0 && path.isAbsolute(dispatch49) && (await fs.pathExists(dispatch49)),
+ render49.status === 0 && dispatch49 !== '' && path.isAbsolute(dispatch49) && (await fs.pathExists(dispatch49)),
'installer-produced dev-auto tree renders and dispatches end to end',
- `${render49.stdout}${render49.stderr}`,
+ `${render49.stdout ?? ''}${render49.stderr ?? ''}`,
);📝 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.
| const render49 = spawnSync( | |
| 'uv', | |
| ['run', '--python', '3.11', path.join(scripts49, 'render_skill.py'), '--project-root', root49, '--skill', skill49], | |
| { encoding: 'utf8' }, | |
| ); | |
| const dispatch49 = render49.stdout.trim().replace(/^read and follow /, ''); | |
| assert( | |
| render49.status === 0 && path.isAbsolute(dispatch49) && (await fs.pathExists(dispatch49)), | |
| 'installer-produced dev-auto tree renders and dispatches end to end', | |
| `${render49.stdout}${render49.stderr}`, | |
| ); | |
| const render49 = spawnSync( | |
| 'uv', | |
| ['run', '--python', '3.11', path.join(scripts49, 'render_skill.py'), '--project-root', root49, '--skill', skill49], | |
| { encoding: 'utf8' }, | |
| ); | |
| const dispatch49 = (render49.stdout || '').trim().replace(/^read and follow /, ''); | |
| assert( | |
| render49.status === 0 && dispatch49 !== '' && path.isAbsolute(dispatch49) && (await fs.pathExists(dispatch49)), | |
| 'installer-produced dev-auto tree renders and dispatches end to end', | |
| `${render49.stdout ?? ''}${render49.stderr ?? ''}`, | |
| ); |
🤖 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 `@test/test-installation-components.js` around lines 3683 - 3693, Guard the
`render49.stdout` access when deriving `dispatch49`, using an empty-string
fallback before calling `.trim()` so unavailable `uv` still reaches the existing
assertion. Keep the `render49.status` check and
`${render49.stdout}${render49.stderr}` diagnostics unchanged.
1e73add to
a956bc5
Compare
Render complete dev-auto workflows into root-scoped immutable snapshots using shared declarative rendering and strict TOML configuration layers. Keep generated render state out of installer module discovery and custom file preservation, preserve quick-dev behavior, and provide deterministic Python version failures for standalone resolver use.
a956bc5 to
0328e92
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
What
Restore Dev Auto rendering as one installed tool call that publishes complete, inspectable, immutable workflow snapshots scoped by project root and effective inputs.
Why
The previous restoration direction duplicated renderer logic and relied on mutable publication. This keeps strict validation and inspectable output without coupling Dev Auto to Quick Dev or allowing generated render state to leak into installer module and custom-file handling.
How
uv run --no-cache, require Python 3.11+, and avoid bytecode beside installed scripts.Testing
HUSKY=0 npm ci && npm run qualitygpt-5.6-sol/high-reasoning Dev Auto E2Es covering JavaScript routing and Python concurrent caching; both passed hidden acceptance, clean-tree, canonical revision, and immutable snapshot-reuse checks--no-cacheruns the exact renderer without home-cache access; version-controlled Dev Auto still requires permission to update repository metadata