Conversation
* docs: align core docs and sync pending changes * fix: preserve partial staging in markdown autofix hook --------- Co-authored-by: Dominikus Nold <djm81@users.noreply.github.com>
Co-authored-by: Dominikus Nold <djm81@users.noreply.github.com>
…g migration to module
* refactor: remove backlog ownership from core cli * fix: align CI marketplace validation paths * test: stabilize command audit validation and add command-surface change --------- Co-authored-by: Dominikus Nold <djm81@users.noreply.github.com>
# Conflicts: # openspec/CHANGE_ORDER.md # openspec/changes/module-migration-10-bundle-command-surface-alignment/proposal.md
* fix: finalize cli runtime validation regressions * test: align satisfied dependency logging assertions --------- Co-authored-by: Dominikus Nold <djm81@users.noreply.github.com>
* feat: add OpenSpec change for backlog-core commands migration Change: backlog-02-migrate-core-commands - Add proposal, design, tasks, specs - Add TDD_EVIDENCE.md with implementation progress - GitHub Issue: #389 Rules applied: AGENTS.md Git Worktree Policy, TDD Hard Gate Made-with: Cursor * docs: update TDD_EVIDENCE and tasks for quality gate results Made-with: Cursor * docs: update TDD_EVIDENCE with test fix results Made-with: Cursor * docs: update TDD_EVIDENCE with all test fixes complete Made-with: Cursor --------- Co-authored-by: Dominikus Nold <djm81@users.noreply.github.com>
* fix: use POST instead of PATCH for ADO work item creation Azure DevOps API requires POST (not PATCH) for creating work items. Also fixed category grouping to always register group commands. Made-with: Cursor * docs: add changelog entry for ADO POST fix Made-with: Cursor * chore: bump version to 0.40.4 Made-with: Cursor * fix: update test mocks from PATCH to POST for ADO create - Reverted incorrect unconditional _mount_installed_category_groups call - Updated test_create_issue mocks to use requests.post instead of requests.patch Made-with: Cursor * test: skip category group test when bundles not installed The test_bootstrap_with_category_grouping_disabled_registers_flat_commands test expects bundles like specfact-codebase to be installed, but in CI they may not be. Added pytest.skip() when 'code' command is not available. Made-with: Cursor --------- Co-authored-by: Dominikus Nold <djm81@users.noreply.github.com>
- Archived backlog-02-migrate-core-commands change - Updated CHANGE_ORDER.md with implementation status - Updated main specs with backlog-add, backlog-analyze-deps, backlog-delta, backlog-sync, backlog-verify-readiness Made-with: Cursor
* feat: document code-review module scaffold * chore: sync 0.41.0 release version artifacts --------- Co-authored-by: Dominikus Nold <djm81@users.noreply.github.com>
Signed-off-by: Dom <39115308+djm81@users.noreply.github.com>
SpecFact CLI Validation Report✅ All validations passed! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6b57a8979
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/specfact_cli/utils/progressive_disclosure.py (1)
277-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--help-advancednow leaks hidden-option state across later invocations.
_patched_get_params()flipsparam.hidden = Falsein place, and this change installs that code path onto the global Click/Typer command classes. After one advanced-help render in the same process, later plain--helpcalls can inherit permanently unhidden params. Restore the originalhiddenflags after rendering, or avoid mutating the shared param objects at all.Also applies to: 456-467
🤖 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/specfact_cli/utils/progressive_disclosure.py` around lines 277 - 303, _patched_get_params currently mutates shared Click/Typer Parameter objects by setting param.hidden = False when _is_advanced_help_context(ctx) is true, which leaks state across invocations; change the implementation to avoid mutating shared objects by creating per-call copies or by recording original hidden flags and restoring them after rendering: obtain all_params from self.params and help_option as now, but for the advanced-help branch make shallow copies of params (or save a list of original_hidden = [getattr(p,"hidden",False) for p in all_params]) and return/operate on the copies (or reset each param.hidden to original_hidden afterwards) so that the global param.hidden flags on the Command class are never permanently changed by _patched_get_params.src/specfact_cli/utils/env_manager.py (1)
361-382:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProbe the managed environment before falling back to global PATH.
check_tool_in_env()still returnsTrueas soon asshutil.which(tool_name)succeeds, so a globally installed binary can hide the fact that the activeuv/hatchenvironment does not provide the tool. That defeats the new “prefer active runner” behavior and can reintroduce the stale-manager diagnostics this slice is meant to fix.Suggested reordering
- # First check if tool is globally available - if shutil.which(tool_name) is not None: - return True, None - if env_info.available and env_info.command_prefix: probe_command = build_tool_command(env_info, [tool_name, "--version"]) try: result = subprocess.run( probe_command, @@ detail = (result.stderr or result.stdout or "").strip() suffix = f": {detail}" if detail else "" return False, f"Tool '{tool_name}' not available in {env_info.manager.value} environment{suffix}" + + if shutil.which(tool_name) is not None: + return True, NoneAs per coding guidelines,
openspec/changes/tester-cli-reliability/specs/runtime-tool-probing/spec.md: "Diagnostics must prefer the active execution context over stale global package-manager inventory."🤖 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/specfact_cli/utils/env_manager.py` around lines 361 - 382, Reorder probing so the active managed environment is checked before falling back to global PATH: in check_tool_in_env(), if env_info.available and env_info.command_prefix, run the existing probe using build_tool_command(...) and subprocess.run(...) and return True/False based on that result (including the current error/detail messages); only if the env probe is not possible (env_info not available or no command_prefix) and/or the probe indicates the tool is not present should you then call shutil.which(tool_name) and return True if found globally. Ensure you reference the same symbols: check_tool_in_env, env_info, build_tool_command, subprocess.run, and shutil.which..github/workflows/pr-orchestrator.yml (1)
324-328:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd the packaged
pip wheelleg before treating this runtime matrix as spec-complete.This launcher set still exercises editable pip installs, not the wheel-install path the release gate requires. That means this workflow can pass without validating the packaged install surface that OpenSpec marks as blocking for CLI runtime/docs changes.
As per coding guidelines,
openspec/changes/tester-cli-reliability/specs/ci-integration/spec.md: "CI blocking requirement: PRs that change CLI runtime/docs/validators/module discovery/smoke scripts must run command-contract smoke tests through a package-manager runtime matrix (hatch, pip wheel install, pipx install, uv run, uv tool/uvx)."🤖 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 @.github/workflows/pr-orchestrator.yml around lines 324 - 328, The runtime-discovery smoke test currently omits the packaged wheel install path; update the "Runtime discovery smoke test" job by adding the packaged wheel launcher to the command that invokes scripts/runtime_discovery_smoke.py (e.g. include --launcher pip-wheel or the project’s canonical pip-wheel launcher name alongside --launcher pip-editable and --launcher pipx) so the workflow exercises the pip wheel install surface before marking this runtime matrix as spec-complete.
🟠 Major comments (22)
src/specfact_cli/cli.py-574-575 (1)
574-575:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
_is_group_like()so it guarantees the.commandsattribute that later code dereferences.
_is_group_like()currently only checkslist_commands/get_command(i.e.,click.MultiCommand-like), but later logic assumes aclick.Group-like interface:
_LazyDelegateGroup._get_real_click_group()castsclick_cmdtoclick.Groupwhen_is_group_like(click_cmd)is true, and_LazyDelegateGroup.list_commands()then readsreal_group.commands.keys()._flatten_specfact_nested_subgroup()pops a nested command intoredundant, and if_is_group_like(redundant)is true it iteratesredundant.commands.items()—which will crash for multi-command implementations that don’t expose.commands.Possible tightening
def _is_group_like(command: object) -> bool: - return hasattr(command, "list_commands") and hasattr(command, "get_command") + return isinstance(command, click.Group) or ( + hasattr(command, "list_commands") + and hasattr(command, "get_command") + and hasattr(command, "commands") + )🤖 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/specfact_cli/cli.py` around lines 574 - 575, _is_group_like currently returns True for objects with list_commands/get_command but later code (in _LazyDelegateGroup._get_real_click_group, _LazyDelegateGroup.list_commands and _flatten_specfact_nested_subgroup) expects a Group-like .commands mapping; update _is_group_like(command) to also require that the object exposes a .commands attribute (and that it is mapping-like, e.g., supports .items()/.keys()) so only true Group-like objects pass the test; reference _is_group_like, _LazyDelegateGroup._get_real_click_group, _LazyDelegateGroup.list_commands and _flatten_specfact_nested_subgroup when making this change..github/workflows/docs-review.yml-22-26 (1)
22-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the
check-command-overviewvalidator source to workflow path triggers.The workflow now enforces
hatch run check-command-overview(Line 119), but changes to the validator source itself are not part of the trigger paths. That can silently skip this CI gate when its logic changes.Suggested patch
pull_request: paths: - "scripts/check-docs-commands.py" + - "scripts/check-command-overview.py" - "scripts/check-command-contract.py" - "scripts/generate-command-overview.py" - "docs/reference/commands.generated.*" - "llms.txt" @@ push: paths: - "scripts/check-docs-commands.py" + - "scripts/check-command-overview.py" - "scripts/check-command-contract.py" - "scripts/generate-command-overview.py" - "docs/reference/commands.generated.*" - "llms.txt"As per coding guidelines,
.github/workflows/**must maintain CI safety and alignment with required validation gates.Also applies to: 50-54
🤖 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 @.github/workflows/docs-review.yml around lines 22 - 26, The workflow's path triggers are missing the validator source for the new check-command-overview job, so changes to the validator code can bypass the CI gate; update the workflow's paths list (the array that currently contains "scripts/check-docs-commands.py", "scripts/check-command-contract.py", "scripts/generate-command-overview.py", "docs/reference/commands.generated.*", "llms.txt") to also include the new validator source (e.g., "scripts/check-command-overview.py" or the actual validator file name) so that modifications to the check-command-overview validator will trigger the docs-review workflow; ensure the same addition is applied to the other similar path groups referenced around the 50-54 region..github/workflows/specfact.yml-112-116 (1)
112-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid
${{ ... }}interpolation inside the bash script forbudgetinspecfact.ymlIn
.github/workflows/specfact.yml(budget='${{ steps.validation.outputs.budget }}'), the workflow expression is rendered into the shell before the^[0-9]+$check. Forworkflow_dispatch,steps.validation.outputs.budgetis derived from user-providedINPUT_BUDGET, so a quote-breaking payload could execute commands prior to validation.Suggested patch
- name: Run Contract Validation id: repro continue-on-error: true + env: + VALIDATION_BUDGET: ${{ steps.validation.outputs.budget }} run: | - budget='${{ steps.validation.outputs.budget }}' + budget="${VALIDATION_BUDGET}" if ! [[ "$budget" =~ ^[0-9]+$ ]]; then echo "Invalid budget: $budget" >&2 echo "exit_code=2" >> "$GITHUB_OUTPUT" exit 0 fi🤖 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 @.github/workflows/specfact.yml around lines 112 - 116, The run step is unsafe because the workflow expression is directly interpolated into the shell line (budget='${{ steps.validation.outputs.budget }}'), allowing a quote-breaking payload to execute before validation; fix it by passing the value through an environment variable (e.g. set env: BUDGET: ${{ steps.validation.outputs.budget }}) and then in the script read budget="$BUDGET" and perform the numeric regex check (if ! [[ "$budget" =~ ^[0-9]+$ ]]; then ...). Update the step to reference the env var instead of embedding the ${{ ... }} expression in the shell command so validation runs on the untrusted input before any shell interpretation.docs/core-cli/modes.md-232-233 (1)
232-233:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
code importargument ordering to match the enforced command contract.These examples place
--repobefore the positional bundle name (my-project), which conflicts with the required canonical invocation and will fail docs-command validation.Suggested fix
-hatch run specfact code import --repo . my-project --confidence 0.7 +hatch run specfact code import my-project --repo . --confidence 0.7 ... -hatch run specfact code import --repo . my-project --confidence 0.7 +hatch run specfact code import my-project --repo . --confidence 0.7As per coding guidelines, "docs/**/*.md: User-facing accuracy: CLI examples match current behavior" and OpenSpec requires validators to reject invalid
specfact code import <bundle> --repo ...ordering.Also applies to: 243-244
🤖 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/core-cli/modes.md` around lines 232 - 233, Update the CLI examples for the specfact command so the positional bundle name comes immediately after the subcommand and before any flags: change occurrences of "specfact code import --repo . my-project --confidence 0.7" (and the similar example at the later occurrence) to the canonical ordering "specfact code import my-project --repo . --confidence 0.7" so the invocation of specfact code import <bundle> matches the enforced command contract and will pass docs-command validation.docs/examples/dogfooding-specfact-cli.md-34-35 (1)
34-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse canonical
code importordering in both workflow snippets.Both snippets put
--repobefore the positional bundle name, which is the legacy ordering that validators are expected to reject.Suggested fix
-specfact code import --repo . specfact-cli --confidence 0.5 +specfact code import specfact-cli --repo . --confidence 0.5 ... -specfact code import --repo . specfact-cli --confidence 0.5 +specfact code import specfact-cli --repo . --confidence 0.5As per coding guidelines, "docs/**/*.md: User-facing accuracy: CLI examples match current behavior" and OpenSpec requires rejecting invalid
specfact code importoption placement.Also applies to: 541-542
🤖 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/examples/dogfooding-specfact-cli.md` around lines 34 - 35, The CLI examples use legacy option ordering for the "specfact code import" command; update both workflow snippets so the positional bundle name comes immediately after "specfact code import" and place flags like "--repo" and "--confidence 0.5" after that positional argument (i.e., "specfact code import <bundle> --repo . --confidence 0.5"), and apply the same change to the other snippet mentioned (lines near 541-542) so examples match the current validator-accepted ordering.docs/examples/integration-showcases/integration-showcases-testing-guide.md-1157-1159 (1)
1157-1159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign baseline verification command flag with the documented command API.
This step uses
--project-name, but the guide’s own health-check examples use--bundle. If--project-nameis not a valid option here, this step breaks the pre-commit baseline workflow.Suggested fix
-specfact --no-banner project health-check --project-name "$BUNDLE_NAME" --repo . +specfact --no-banner project health-check --bundle "$BUNDLE_NAME" --repo .As per coding guidelines, "docs/**/*.md: User-facing accuracy: CLI examples match current behavior."
🤖 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/examples/integration-showcases/integration-showcases-testing-guide.md` around lines 1157 - 1159, The health-check example uses an invalid flag --project-name; update the CLI call so it matches the documented/implemented API by replacing --project-name with --bundle (i.e., use specfact ... health-check --bundle "$BUNDLE_NAME" --repo .) so the specfact health-check invocation, the BUNDLE_NAME variable, and the docs align with the actual command signature.src/specfact_cli/modules/init/src/commands.py-714-718 (1)
714-718:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
initcallback dropped runtime type-contract enforcement.
initis a public CLI entrypoint but no longer has@beartype. Please restore it to keep public API contract checks consistent with repo policy.As per coding guidelines, “All public APIs must have `@icontract` decorators and `@beartype` type checking”.✅ Minimal fix
`@app.callback`(invoke_without_command=True) `@require`(lambda repo: _is_valid_repo_path(repo), "Repo path must exist and be directory") `@ensure`(lambda result: result is None, "Command should return None") +@beartype def init( ctx: typer.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 `@src/specfact_cli/modules/init/src/commands.py` around lines 714 - 718, The init CLI callback lost runtime type checking—add the `@beartype` decorator back onto the init function (above the existing `@app.callback/`@require/@ensure decorators as per project convention) and ensure there is a matching import "from beartype import beartype" at the top of the module; keep the existing `@require` and `@ensure` decorators intact so the function retains its icontract checks and public API contract enforcement.scripts/check-docs-commands.py-248-253 (1)
248-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGenerated-contract validation still accepts unknown subcommands.
Line 252/Line 270 currently pass any command that has any known prefix, so invalid paths can slip through (e.g., a typo subcommand under a valid root). That weakens the docs contract gate.
Proposed fix
-def _generated_prefix_match(tokens: list[str]) -> bool: +def _generated_prefix_match(tokens: list[str]) -> tuple[bool, str]: generated = _generated_command_prefixes() if not generated: - return False - return any(tuple(tokens[:size]) in generated for size in range(len(tokens), 0, -1)) + return False, "" + for size in range(len(tokens), 0, -1): + prefix = tuple(tokens[:size]) + if prefix not in generated: + continue + has_children = any(len(cmd) > size and cmd[:size] == prefix for cmd in generated) + if has_children and size < len(tokens): + next_token = tokens[size] + if not next_token.startswith("-") and not re.match(r"^<[^>]+>$", next_token): + return False, f"Unknown subcommand `{next_token}` after `{' '.join(prefix)}`." + return True, "" + return False, "" @@ - if _generated_prefix_match(tokens): - return True, "" + generated_ok, generated_msg = _generated_prefix_match(tokens) + if generated_ok: + return True, "" + if generated_msg: + return False, generated_msgAs per coding guidelines,
openspec/changes/tester-cli-reliability/specs/generated-command-overview/spec.mdrequires docs/guidance validation to use the generated command contract and fail obsolete/invalid commands.Also applies to: 270-278
🤖 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 `@scripts/check-docs-commands.py` around lines 248 - 253, The _generated_prefix_match currently returns True if any prefix of tokens exists in the generated set, which lets invalid subcommands slip through; change the logic so it only accepts an exact match of the full token sequence (i.e., return tuple(tokens) in generated) and remove the prefix-any check, and apply the same fix to the analogous prefix-check block later in the file (the other loop that iterates sizes around lines 270-278) so only complete generated command tuples are allowed.scripts/generate-command-overview.py-50-66 (1)
50-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winModule import bootstrap is non-deterministic when multiple module repos are present.
The loop adds all valid candidates to
sys.path. If multiple candidates exist, command ownership and discovered command trees can vary by environment, which undermines deterministic generated artifacts.Proposed fix
- for candidate in module_repo_candidates: + for candidate in module_repo_candidates: if candidate is None: continue packages_dir = candidate / "packages" if not packages_dir.is_dir(): continue os.environ.setdefault("SPECFACT_MODULES_REPO", str(candidate.resolve())) for src_path in sorted(packages_dir.glob("*/src")): src = str(src_path.resolve()) if src not in sys.path: sys.path.insert(0, src) + breakAs per coding guidelines,
openspec/changes/tester-cli-reliability/specs/generated-command-overview/spec.mdrequires deterministic generation from the actual command tree for the same source tree.🤖 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 `@scripts/generate-command-overview.py` around lines 50 - 66, The bootstrap loop indiscriminately adds all valid entries from module_repo_candidates into sys.path, causing non-deterministic command discovery; change the logic to pick the first valid candidate only: iterate module_repo_candidates, skip None, check packages_dir.is_dir(), then set SPECFACT_MODULES_REPO to that candidate, add its packages/*/src entries to sys.path (preserving insertion order) and break out of the loop so no other candidates are processed; keep the existing REPO_ROOT / "src" handling unchanged.docs/reference/commands.md-169-169 (1)
169-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReference example should match canonical
code importcontract syntax.This line should use the exact invocation shape required by the generated command contract (including subcommand/argument order) to avoid docs-validator failures and user confusion.
As per coding guidelines, docs command examples must be validated against the generated command overview 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 `@docs/reference/commands.md` at line 169, The example invocation currently uses incorrect argument order for the code import command; update the docs line to match the canonical `code import` contract by placing the positional subcommand/argument before flags (e.g. change `specfact code import --repo . legacy-api` to `specfact code import legacy-api --repo .`), then re-run the docs/command validator against the generated command overview to confirm it matches the contract for `code import`.docs/reference/commands.generated.md-13-121 (1)
13-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGenerated command overview Markdown is stale and must be regenerated.
The docs-review pipeline flags this file as out-of-date (notable option-list deltas). Please regenerate from the same source inventory used for
commands.generated.jsonso both artifacts stay deterministic and consistent.As per coding guidelines, “The same inventory data must drive generated command overview artifacts and validators (no separate manually maintained allowlist)” and freshness checks must fail on drift.
🤖 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/reference/commands.generated.md` around lines 13 - 121, The generated docs file docs/reference/commands.generated.md is stale and must be regenerated from the same inventory source used for commands.generated.json; run the command-generation tool (the generator that produces commands.generated.json) to re-create commands.generated.md so both artifacts are identical and deterministic, remove any manual edits to commands.generated.md, and commit the regenerated file; if the generator or CI step is missing/broken, update the generator invocation used in the docs pipeline (the script that writes commands.generated.json and commands.generated.md) so freshness checks will pass and no manual allowlist is kept.docs/reference/commands.generated.json-1-2185 (1)
1-2185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGenerated command contract JSON is stale relative to runtime command tree.
CI already reports this artifact is out-of-date (e.g.,
bare_invocationand option-set diffs). Regenerate and commit the JSON from the current CLI tree before merge, otherwise command-contract checks and downstream docs validation remain unreliable.As per coding guidelines, “Freshness must be enforced: command-overview ‘check’ must fail when inputs change and must tell the developer which generator command refreshes artifacts.”
🤖 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/reference/commands.generated.json` around lines 1 - 2185, The commands.generated.json artifact is stale (fields like bare_invocation and option sets mismatch runtime CLI); regenerate the JSON from the current CLI tree using the same generator that CI uses (run the command-overview check/refresh generator referenced by your CI job), verify the new commands.generated.json matches (no diffs for bare_invocation/option sets), and commit the regenerated file; check the CLI entrypoints (e.g., specfact_cli.cli:app) if the generator needs to be pointed at the correct app.src/specfact_cli/utils/bundle_loader.py-136-136 (1)
136-136:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMigration hint appears to reference a non-existent command path.
specfact project migrate artifacts ...does not appear in the generated command inventory included in this PR. If this hint is wrong, users hit a dead-end during recovery from format errors. Please update this message to a verified command 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/specfact_cli/utils/bundle_loader.py` at line 136, The migration hint appended to the error_msg in bundle_loader.py references an incorrect command path ("specfact project migrate artifacts ..."); locate the code that builds error_msg (the error_msg variable in bundle_loader.py) and replace that hint with the actual, verified CLI command from the current command inventory (or remove the hint if no migration command exists). Ensure the updated string uses the correct command syntax from your CLI registry (e.g., the exact command name and flags) so users are directed to a working recovery command.src/specfact_cli/utils/structure.py-264-264 (1)
264-264:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLegacy migration error hint likely points to an invalid command.
specfact project migrate artifacts --repo .appears inconsistent with the command inventory shipped in this PR. Please update to a verified migration command to avoid blocking users on legacy-config recovery.🤖 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/specfact_cli/utils/structure.py` at line 264, The error message string in src/specfact_cli/utils/structure.py that currently reads "Please migrate to the new bundle structure using 'specfact project migrate artifacts --repo .'." references a non-existent CLI invocation; update that literal to the verified migration command provided by this PR’s CLI (replace the quoted command with the correct one from the command inventory), ensuring you change the string where it appears in structure.py so users are directed to the actual migration command (search for the exact legacy message text to locate the spot).src/specfact_cli/utils/structure.py-980-980 (1)
980-980:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
create_readme()emits command examples that don’t match the current CLI tree.The generated README uses
specfact project plan ...examples, but current command surfaces in this PR arespecfact project <subcommand>(e.g.,devops-flow,snapshot, etc.). This will generate broken onboarding guidance for users.As per coding guidelines, user-facing CLI examples must match current behavior.
Also applies to: 986-986
🤖 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/specfact_cli/utils/structure.py` at line 980, The README generation in create_readme() currently emits outdated CLI examples using the command prefix "specfact project plan ..." which no longer matches the updated CLI surface (commands under "specfact project <subcommand>" such as "devops-flow" and "snapshot"); update create_readme() to replace any hardcoded "specfact project plan" examples with the correct dynamic subcommand usage (e.g., "specfact project devops-flow", "specfact project snapshot" or generate examples from the current command tree) so the user-facing examples reflect the actual CLI functions exposed by the project.docs/guides/troubleshooting.md-120-120 (1)
120-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the canonical
code importinvocation shape in troubleshooting examples.These examples use a
specfact code import --repo . legacy-api ...form that can drift from the command-contract validator expectations and break docs validation. Please align these lines to the canonicalcode importshape used by the generated contract (including required subcommand/argument ordering where applicable).As per coding guidelines, “Docs command examples in reference/prompt/template/guidance strings must be validated against the generated command overview contract” and “Invalid
specfact code importoption placement must fail; validators must report the canonical supported invocation.”Also applies to: 126-126, 144-144, 259-259, 355-355, 462-462, 486-486, 493-493
🤖 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/guides/troubleshooting.md` at line 120, The command example uses the wrong argument order: replace instances of "specfact code import --repo . legacy-api --verbose" with the canonical invocation "specfact code import legacy-api --repo . --verbose" (i.e., move the positional repo name immediately after "import" and keep flags after) and similarly update the other reported occurrences of the "specfact code import" examples to follow this canonical argument/subcommand ordering so they validate against the generated command contract.llms.txt-15-123 (1)
15-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegenerate
llms.txt; current command contract artifact is stale and CI-blocking.CI already reports this generated file is out of date, so the checked-in contract is not trustworthy for validation/promotions in this PR state. Please re-run the command-overview generator and commit the refreshed artifact before merge.
As per coding guidelines, "Freshness must be enforced: command-overview 'check' must fail when inputs change and must tell the developer which generator command refreshes artifacts."
🤖 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 `@llms.txt` around lines 15 - 123, The checked-in llms.txt is stale—re-run the command-overview generator to refresh the artifact (run the project's command-overview generator task), replace the committed llms.txt with the regenerated output, commit the updated file, and push so CI can validate; ensure the generator invocation referenced by the repo's command-overview/check pipeline is the one you ran so the CI 'check' will pass and report the correct refresh command if inputs change.src/specfact_cli/utils/ide_setup.py-430-436 (1)
430-436:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkill path derivation can silently overwrite exports from different module owners.
_source_id_to_skill_name()drops the owner/namespace and keeps only the trailing token, so distinct sources can map to the same output path (e.g.,*/specfact-backlog→specfact-backlog/SKILL.md). In that case one source overwrites another during export.Suggested fix
def _source_id_to_skill_name(source_id: str) -> str: """Map a prompt source id to a capability-oriented skill directory name.""" if source_id == PROMPT_SOURCE_CORE: return "specfact-cli" - raw_name = source_id.strip().split("/")[-1] or source_id.strip() - normalized = re.sub(r"[^A-Za-z0-9]+", "-", raw_name).strip("-").lower() + # Keep namespace information to avoid cross-owner collisions. + raw_name = source_id.strip().replace("/", "-") + normalized = re.sub(r"[^A-Za-z0-9]+", "-", raw_name).strip("-").lower() return normalized or "specfact-cli"🤖 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/specfact_cli/utils/ide_setup.py` around lines 430 - 436, _source_id_to_skill_name currently drops the owner/namespace and only keeps the trailing token, causing different source_ids to collide; update _source_id_to_skill_name to preserve uniqueness by incorporating the full source_id (or at least owner + name) into the generated directory name: keep the PROMPT_SOURCE_CORE special-case, otherwise take the full source_id (or join owner and raw_name if you parse by "/"), sanitize it with the existing re.sub to replace non-alphanumerics with "-" and lowercase, and fall back to "specfact-cli" only if the result is empty; this ensures distinct owners produce distinct skill names and prevents silent overwrites.scripts/runtime_discovery_smoke.py-347-354 (1)
347-354:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelax brittle parent-help token checks in smoke validation.
Line 348 hard-fails when
specfact code import --helpomitsfrom-code/from-bridge, but your CI failures show this can vary by launcher while leaf help still works. This turns a smoke check into a false blocker.Proposed fix
def _assert_code_help(cli: list[str], demo: Path, env: dict[str, str]) -> None: @@ _run([*cli, "code", "review", "run", "--help"], cwd=demo, env=env) import_help = _run([*cli, "code", "import", "--help"], cwd=demo, env=env) - _assert_tokens_present(import_help.stdout, "specfact code import --help", ("from-code", "from-bridge")) + if "specfact code import" not in _normalized_output(import_help.stdout): + raise AssertionError("`specfact code import --help` did not render import command help") from_code_help = _run([*cli, "code", "import", "from-code", "--help"], cwd=demo, env=env) - if "from-code [OPTIONS]" not in _normalized_output(from_code_help.stdout): + if "from-code" not in _normalized_output(from_code_help.stdout): raise AssertionError("`specfact code import from-code --help` did not render the explicit subcommand") from_bridge_help = _run([*cli, "code", "import", "from-bridge", "--help"], cwd=demo, env=env) - if "from-bridge [OPTIONS]" not in _normalized_output(from_bridge_help.stdout): + if "from-bridge" not in _normalized_output(from_bridge_help.stdout): raise AssertionError("`specfact code import from-bridge --help` did not render the explicit subcommand")🤖 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 `@scripts/runtime_discovery_smoke.py` around lines 347 - 354, The parent-help token assertion in the smoke test is too brittle: replace the hard-fail call to _assert_tokens_present on import_help (the check for "from-code"/"from-bridge") with a relaxed check that does not raise CI failures — e.g. attempt to assert tokens exist but catch/handle the failure (or log a warning) and allow the test to continue to the explicit subcommand validations where from_code_help and from_bridge_help are checked via _normalized_output; update code around import_help, _assert_tokens_present, from_code_help, from_bridge_help and _normalized_output to implement this non-fatal behavior.scripts/check-command-contract.py-82-87 (1)
82-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on malformed command records instead of silently skipping them.
Line 86 drops non-dict entries, which can mask contract corruption and give a false green validation result. This validator should reject malformed artifacts explicitly.
Suggested fix
def _load_records() -> list[dict[str, Any]]: raw = json.loads(COMMANDS_JSON.read_text(encoding="utf-8")) if not isinstance(raw, list): raise ValueError(f"{COMMANDS_JSON} must contain a JSON list") - return [entry for entry in raw if isinstance(entry, dict)] + records: list[dict[str, Any]] = [] + for index, entry in enumerate(raw): + if not isinstance(entry, dict): + raise ValueError( + f"{COMMANDS_JSON} entry at index {index} must be an object, got {type(entry).__name__}" + ) + records.append(entry) + return records🤖 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 `@scripts/check-command-contract.py` around lines 82 - 87, The _load_records function currently silently drops non-dict entries which can hide corrupted command records; change it to fail fast by validating that raw is a list and then iterating raw to ensure every entry is a dict (referencing _load_records and COMMANDS_JSON) and raise a clear ValueError if any entry is not a dict (include index and a brief representation of the offending entry in the error message) instead of filtering them out so malformed artifacts cause explicit validation failure.resources/bundled-module-registry/index.json-4-7 (1)
4-7:⚠️ Potential issue | 🟠 MajorFix registry
download_urlassets (init/upgrade) so SHA-256 integrity can be checkedIn
resources/bundled-module-registry/index.json, both referenced tarballs for:
init(https://github.com/nold-ai/specfact-cli/releases/download/init-0.1.36.tar.gz, sha25685ce7a1aeed33844823878bac36b18dc2364c7506ad6ab28b77c42b5f0400fac)upgrade(https://github.com/nold-ai/specfact-cli/releases/download/upgrade-0.1.21.tar.gz, sha25635c592374b209224af9a826c21efa66947cc16bf680b20ee0463f44bee4f9b59)return HTTP 404 here, so the “download → compute SHA-256 → compare” integrity verification can’t be completed and consumers can’t fetch the artifacts. Update/publish the release assets or correct the
download_urlvalues, then rerun the SHA-256 comparison against the resolved tarballs.🤖 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 `@resources/bundled-module-registry/index.json` around lines 4 - 7, The registry entries in resources/bundled-module-registry/index.json for id "init" and "upgrade" contain download_url values that return 404, so integrity checks fail; fix this by either uploading the correct tarball assets to those GitHub release pages or replacing the download_url fields for "init" and "upgrade" with the correct public URLs, then re-download the resolved tarballs and recompute their SHA-256 checksums and update the corresponding checksum_sha256 fields to the new values.src/specfact_cli/__init__.py-56-76 (1)
56-76:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove progressive-disclosure bootstrap out of import-time and fail closed on loader resolution errors.
Running
_install_progressive_disclosure()during module import makes package import behavior depend on dynamic loader/file execution, and thespec is None or spec.loader is Noneearly return can silently skip the shared CLI error contract. Trigger this bootstrap explicitly in CLI startup instead, and raise a clear error if bootstrap cannot be loaded.As per coding guidelines,
No side-effects at import time in src/ — no network calls, no file I/O on module load.🤖 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/specfact_cli/__init__.py` around lines 56 - 76, The progressive-disclosure bootstrap is being run at import time; remove the unconditional call to _install_progressive_disclosure() at module import and instead make _install_progressive_disclosure() fail closed by raising a clear exception when importlib.util.spec_from_file_location returns None or spec.loader is None (replace the early "return" with raising a RuntimeError including module_path), keep the try/except that pops sys.modules but allow the exception to propagate, and ensure the function is exported (or documented) so the CLI startup code invokes _install_progressive_disclosure() explicitly during CLI initialization rather than during module import.
🟡 Minor comments (2)
tests/unit/commands/test_update.py-150-153 (1)
150-153:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis test doesn’t actually exercise the new uv-run branch.
Setting
UV_PROJECT_ENVIRONMENTmakesdetect_installation_method()return from_detect_uv_project_installation()before_detect_uv_run_installation(). The test name/intention and setup are currently mismatched.As per coding guidelines, “tests/**/*.py: Contract-first testing: meaningful scenarios, not redundant assertions already covered by contracts. Flag flakiness, environment coupling, and missing coverage for changed behavior.”🎯 Tighten the test to target uv-run specifically
`@patch.dict`( "specfact_cli.modules.upgrade.src.commands.os.environ", - {"UV_PROJECT_ENVIRONMENT": "/workspace/app/.venv", "UV": "1"}, + {"UV": "1"}, clear=False, )Also applies to: 155-176
🤖 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/unit/commands/test_update.py` around lines 150 - 153, The test currently sets UV_PROJECT_ENVIRONMENT which causes detect_installation_method() to return from _detect_uv_project_installation() and never exercise the uv-run branch; update the test in tests/unit/commands/test_update.py to remove or avoid setting "UV_PROJECT_ENVIRONMENT" in the patched environment and instead arrange the environment/mocks to trigger _detect_uv_run_installation() (for example set the specific uv-run indicator env var or mock filesystem/paths that _detect_uv_run_installation() checks) so the test actually exercises the uv-run behavior and asserts the expected outcome from detect_installation_method().scripts/pre-commit-quality-checks.sh-420-421 (1)
420-421:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProtect generated-artifact staging from unintended local hunks.
git add -- llms.txt ...is unconditional here. If a developer has unrelated unstaged edits in these files, this hook stages them silently. Add the same unstaged-hunk guard pattern used in Markdown auto-fix before staging generated artifacts.🤖 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 `@scripts/pre-commit-quality-checks.sh` around lines 420 - 421, The unconditional staging line "git add -- llms.txt docs/reference/commands.generated.json docs/reference/commands.generated.md" can silently pick up unrelated local hunks; before this line, add the same unstaged-hunk guard used in the Markdown auto-fix flow: detect if any of those three files have unstaged changes (e.g., via git diff --name-only or the project's existing helper) and if so abort or warn and skip staging so only generated changes are staged; update the script to perform that check and only run the git add command when no unstaged hunks are present for those specific files.
🧹 Nitpick comments (2)
src/specfact_cli/modules/init/src/commands.py (1)
616-619: ⚡ Quick winDerive
--idehelp choices fromIDE_CONFIGto avoid drift.This string is now manually enumerating targets; it can diverge from actual supported keys and CLI contract docs. Generate it from
IDE_CONFIG(+auto) instead of hard-coding.As per coding guidelines, “Extract common patterns to avoid code duplication (DRY principle)”.♻️ Suggested refactor
+IDE_TARGETS_HELP = ", ".join([*sorted(IDE_CONFIG.keys()), "auto"]) + ... ide: str | None = typer.Option( None, "--ide", - help=( - "IDE/agent target (cursor, vscode, copilot, claude, claude-skills, codex, mistral, vibe, " - "gemini, qwen, opencode, windsurf, kilocode, auggie, roo, codebuddy, amp, q, auto)" - ), + help=f"IDE/agent target ({IDE_TARGETS_HELP})", ),🤖 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/specfact_cli/modules/init/src/commands.py` around lines 616 - 619, The help text for the --ide option is hard-coded; change it to derive choices from the IDE_CONFIG mapping so it can't drift: import or reference IDE_CONFIG in this module and build the help string dynamically (e.g., join sorted(IDE_CONFIG.keys() + ['auto'])) and use that result in the help= argument where --ide is defined so the displayed choices always reflect IDE_CONFIG plus the 'auto' option; update the symbol referenced in commands.py where the --ide argument is created to use the generated string.tests/unit/docs/test_docs_validation_scripts.py (1)
65-140: ⚡ Quick winAdd one negative test for typo subcommands under generated-contract mode.
The new suite covers option order well, but it should also lock in that unknown subcommands are rejected when
commands.generated.jsonis available.As per coding guidelines,
tests/**/*.pyrequires meaningful scenario coverage for changed behavior rather than leaving contract gaps.🤖 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/unit/docs/test_docs_validation_scripts.py` around lines 65 - 140, Add a negative unit test that verifies unknown/typo subcommands are rejected when generated-contract mode is active: using _load_check_docs_commands() create a test (e.g., test_generated_contract_rejects_unknown_subcommand) that calls mod.validate_command_tokens with tokens representing the generated-contract mode plus a bogus subcommand (for example ["generated-contract", "typo-subcmd", "--help"]) and assert the returned ok is False and the message mentions "generated-contract" and "typo-subcmd"; if needed, monkeypatch mod to simulate presence of commands.generated.json or enable the generated-contract code path so validate_command_tokens exercises the generated-contract validation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5fda445e-0bcf-479b-b655-5a7cbc34a022
📒 Files selected for processing (88)
.github/ISSUE_TEMPLATE/augmentation_metadata.md.github/ISSUE_TEMPLATE/bug_report.md.github/pull_request_template.md.github/workflows/docs-review.yml.github/workflows/pr-orchestrator.yml.github/workflows/specfact.yml.markdownlint.jsonCHANGELOG.mdREADME.mddocs/core-cli/debug-logging.mddocs/core-cli/init.mddocs/core-cli/modes.mddocs/examples/dogfooding-specfact-cli.mddocs/examples/integration-showcases/integration-showcases-quick-reference.mddocs/examples/integration-showcases/integration-showcases-testing-guide.mddocs/examples/integration-showcases/integration-showcases.mddocs/examples/integration-showcases/setup-integration-tests.shdocs/examples/quick-examples.mddocs/getting-started/README.mddocs/getting-started/installation.mddocs/getting-started/quickstart.mddocs/getting-started/tutorial-openspec-speckit.mddocs/getting-started/where-to-start.mddocs/guides/ai-ide-workflow.mddocs/guides/command-chains.mddocs/guides/copilot-mode.mddocs/guides/ide-integration.mddocs/guides/openspec-journey.mddocs/guides/speckit-journey.mddocs/guides/troubleshooting.mddocs/migration/migration-cli-reorganization.mddocs/reference/commands.generated.jsondocs/reference/commands.generated.mddocs/reference/commands.mddocs/reference/directory-structure.mddocs/technical/code2spec-analysis-logic.mdllms.txtopenspec/CHANGE_ORDER.mdopenspec/changes/tester-cli-reliability/.openspec.yamlopenspec/changes/tester-cli-reliability/TDD_EVIDENCE.mdopenspec/changes/tester-cli-reliability/proposal.mdopenspec/changes/tester-cli-reliability/specs/ci-integration/spec.mdopenspec/changes/tester-cli-reliability/specs/cli-error-guidance/spec.mdopenspec/changes/tester-cli-reliability/specs/command-package-runtime-validation/spec.mdopenspec/changes/tester-cli-reliability/specs/core-cli-reference/spec.mdopenspec/changes/tester-cli-reliability/specs/generated-command-overview/spec.mdopenspec/changes/tester-cli-reliability/specs/runtime-tool-probing/spec.mdopenspec/changes/tester-cli-reliability/tasks.mdpyproject.tomlresources/bundled-module-registry/index.jsonresources/templates/pr-template.md.j2scripts/check-command-contract.pyscripts/check-docs-commands.pyscripts/generate-command-overview.pyscripts/pre-commit-quality-checks.shscripts/runtime_discovery_smoke.pysetup.pysrc/__init__.pysrc/specfact_cli/__init__.pysrc/specfact_cli/cli.pysrc/specfact_cli/modules/init/module-package.yamlsrc/specfact_cli/modules/init/src/commands.pysrc/specfact_cli/modules/upgrade/module-package.yamlsrc/specfact_cli/modules/upgrade/src/commands.pysrc/specfact_cli/utils/bundle_loader.pysrc/specfact_cli/utils/env_manager.pysrc/specfact_cli/utils/github_annotations.pysrc/specfact_cli/utils/ide_setup.pysrc/specfact_cli/utils/progressive_disclosure.pysrc/specfact_cli/utils/structure.pysrc/specfact_cli/utils/suggestions.pytests/conftest.pytests/integration/scripts/test_runtime_discovery_smoke.pytests/unit/cli/test_error_guidance.pytests/unit/cli/test_lean_help_output.pytests/unit/commands/test_backlog_daily.pytests/unit/commands/test_update.pytests/unit/docs/test_docs_validation_scripts.pytests/unit/migration/test_module_migration_07_cleanup.pytests/unit/modules/init/test_first_run_selection.pytests/unit/modules/init/test_init_ide_prompt_selection.pytests/unit/registry/test_category_groups.pytests/unit/registry/test_module_installer.pytests/unit/specfact_cli/registry/test_profile_presets.pytests/unit/utils/test_env_manager.pytests/unit/utils/test_ide_setup.pytests/unit/utils/test_suggestions.pytests/unit/workflows/test_trustworthy_green_checks.py
SpecFact CLI Validation Report✅ All validations passed! |
SpecFact CLI Validation Report✅ All validations passed! |
SpecFact CLI Validation Report✅ All validations passed! |
SpecFact CLI Validation Report✅ All validations passed! |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Dom <39115308+djm81@users.noreply.github.com>
SpecFact CLI Validation Report✅ All validations passed! |
Summary
Validation