Harden module command reliability gates#307
Open
djm81 wants to merge 3 commits into
Open
Conversation
|
Strix is installed on this repository, but we could not run this PR security review because this workspace does not have an active plan. If you'd like to continue receiving code reviews, you can add a payment method or manage billing here. |
Contributor
📝 WalkthroughPull Request Summary: Harden Module Command Reliability GatesFor specfact-cli-modules MaintainersBundle & Module Surface: Command Routing & Help ContractsCLI Command Restructuring & Namespacing
Help & Error Messaging Hardening
Adapter & Bridge Runtime Behavior
Manifest & Integrity: Semver & Registry ImpactModule Package Versions: All Patch Bumps
Each module's Module-Level Command Overview Artifacts (New)
Cross-Repo Integration: specfact-cli API Alignment & Dev-DepsPaired Core PR & Compatibility
Workflow Integration & Environment Setup
Code Review Tool Diagnostics
Documentation: Accuracy & URL ContractsCommand Reference & Example Validation (145+ updates)
Project Bundle Documentation Restructuring
Module Prompt & CLI Enforcement Updates
OpenSpec: Change Coverage & Scenario SpecsChange Tracking & Paired Dependencies
Five Specification Modules with Scenario Coverage
Test Evidence & Quality Gates
WalkthroughAdds CLI contract fixes (auth/delta/import/regenerate/semgrep), generates and validates a deterministic command overview, integrates checks into workflows and pre-commit, aligns docs/prompts to canonical commands and import syntax, bumps module versions, and records OpenSpec specs/tasks. Includes extensive unit/e2e tests and workflow updates. ChangesModule CLI contract reliability and behavior fixes
Deterministic command overview, validation scripts, and CI gates
Docs/prompts alignment to canonical commands and help strings
Module version bumps and OpenSpec change tracking
Sequence Diagram(s)(skip) Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
djm81
added
bug
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d639a88c0
ℹ️ 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".
| with: | ||
| repository: nold-ai/specfact-cli | ||
| path: specfact-cli | ||
| ref: ${{ (github.ref == 'refs/heads/main' || github.head_ref == 'main') && 'main' || 'dev' }} |
There was a problem hiding this comment.
For pull_request runs targeting main from a feature branch, this expression checks out dev because GitHub Actions defines github.head_ref as the PR source branch and github.base_ref as the target branch (see GitHub contexts docs). The later steps export this checkout as SPECFACT_CLI_REPO and validate/generated command contracts against it, so main-targeted PRs can pass or fail against the wrong core CLI contract; use the base ref for PRs instead.
Useful? React with 👍 / 👎.
Comment on lines
+106
to
+107
| if is_group and bool(getattr(command, "invoke_without_command", False)): | ||
| return "executes" |
There was a problem hiding this comment.
This treats every invoke_without_command group as a successful bare command, but this same change uses that flag for specfact backlog auth only to print help and exit 2 when no subcommand is provided. As a result docs/reference/commands.generated.json records specfact backlog auth with bare_invocation: executes, and check-command-contract skips the missing-subcommand check for it, leaving the generated contract inaccurate for users and agents.
Useful? React with 👍 / 👎.
Comment on lines
+167
to
+169
| if hatch run generate-command-overview; then | ||
| git add -- llms.txt docs/reference/commands.generated.json docs/reference/commands.generated.md | ||
| success "✅ Command overview artifacts regenerated and staged" |
There was a problem hiding this comment.
When this pre-commit path runs, it regenerates the command overview from the entire working tree and immediately git adds the outputs. If a developer has unstaged command-source or prompt edits while committing only a docs/resource change, the hook stages generated artifacts that reflect those unstaged edits, so the commit can include contract changes the developer did not stage. This gate should either check for staleness or generate from staged content instead of auto-staging working-tree output.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major 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 (1)
.github/workflows/docs-review.yml (1)
97-107:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix failure propagation in “Validate prompt commands” docs gate.
- In
.github/workflows/docs-review.yml(lines ~97-102),python scripts/check-prompt-commands.py 2>&1 | tee ...has nopipefail/PIPESTATUS check afterward, so a failing validator can still result in a successful step (pipeline exit comes fromtee).- The “Validate generated command overview” step (lines ~103-107) doesn’t have a pipeline, so the
PIPESTATUS-based masking claim doesn’t apply; failure should already stop due to defaultbash -e.🤖 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 97 - 107, The "Validate prompt commands" step's pipeline (python scripts/check-prompt-commands.py 2>&1 | tee "$PROMPT_COMMAND_LOG") can mask failures because tee exits successfully; enable failure propagation by running the step under pipefail (e.g., prefix with set -o pipefail or use bash -o pipefail) or explicitly check PIPESTATUS after the pipeline and exit on non-zero (inspect PIPESTATUS[0] from the python command). Also remove or don't apply a PIPESTATUS-style workaround to the "Validate generated command overview" step since it isn't a pipeline—leave it to fail normally under bash -e or ensure no redundant PIPESTATUS handling is present.
🟡 Minor comments (5)
openspec/CHANGE_ORDER.md-44-44 (1)
44-44:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winComplete the source bug range reference (
#586–#592).Line 44 omits
specfact-cli#589while claiming the source bug set spanning#586–#592. Please add#589to keep change-order linkage complete.As per coding guidelines, OpenSpec artifacts must stay truth-aligned with declared proposal/task dependencies and change-order tracking.
🤖 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 `@openspec/CHANGE_ORDER.md` at line 44, The CHANGE_ORDER.md table row for "backlog-core | 00 | tester-module-cli-reliability" currently lists source bugs [specfact-cli#586], [`#587`], [`#588`], [`#590`], [`#591`], [`#592`] but omits `#589`; update that row's source bug list to include [specfact-cli#589](https://github.com/nold-ai/specfact-cli/issues/589) (keeping the same comma-separated/hyperlink format) so the referenced range correctly reads `#586`–#592.packages/specfact-govern/resources/prompts/shared/cli-enforcement.md-114-119 (1)
114-119:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix mismatched command descriptions vs actual invocation.
Line 114, Line 115, Line 117, and Line 118 describe concrete actions but all point to
specfact project --help, which does not perform those actions. This is misleading prompt guidance and can drive invalid automation behavior.As per coding guidelines, user-facing command examples/guidance must remain accurate and aligned with bundled command 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 `@packages/specfact-govern/resources/prompts/shared/cli-enforcement.md` around lines 114 - 119, The prompt lists several CLI examples that incorrectly reference `specfact project --help`; update each misleading entry so the command shown actually performs the described action: replace the `specfact project --help` placeholder for "Initialize project bundle" with the actual initialization command (e.g., `specfact project init` or the project bootstrap command used in the codebase), replace the "Set active plan" example with the real command that sets plans (e.g., `specfact project set-plan <plan-name>`), replace the "Review plan" and "Create SDD manifest" examples with their respective concrete commands (e.g., `specfact project review [<bundle-name>]` and `specfact project create-sdd [<bundle-name>]`), and ensure `specfact code import [<bundle-name>] --repo <path>` and `specfact govern enforce sdd [<bundle-name>]` remain accurate; use the actual CLI verbs used by the project (referencing the command names `specfact project init`, `specfact project set-plan`, `specfact project review`, `specfact project create-sdd`) so examples match real behavior.docs/getting-started/choose-your-modules.md-23-23 (1)
23-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicate
specfact projectentry from the Project command surface.Line 23 lists
specfact projecttwice, which makes the command surface inaccurate.Suggested fix
-| Structure my project, link plans to code, and manage dev lifecycle | **Project** | `specfact project`, `specfact project`, `specfact project sync` | +| Structure my project, link plans to code, and manage dev lifecycle | **Project** | `specfact project`, `specfact project sync` |As per coding guidelines,
docs/**/*.md: “User-facing and cross-site accuracy… CLI examples matching bundled 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 `@docs/getting-started/choose-your-modules.md` at line 23, The docs line contains a duplicated CLI entry: remove the duplicate `specfact project` so the command surface reads only once; update the table cell that currently lists "`specfact project`, `specfact project`, `specfact project sync`" to list "`specfact project`, `specfact project sync`" (ensuring spacing/commas match adjacent rows) so the Project command surface accurately reflects available commands.scripts/check-prompt-commands.py-377-377 (1)
377-377:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDefault-path help text does not match implementation
Line 377 claims defaults include
packages/*/src/**/*.py, but Line 384 only returns resource paths. Either update the help text or include source path discovery in the no-arg path selection to prevent silent coverage gaps.Also applies to: 384-384
🤖 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-prompt-commands.py` at line 377, The help text for the paths option claims defaults include "packages/*/src/**/*.py" but the no-argument path-selection code only returns package resource paths, causing a mismatch; either update the help string to accurately state the real defaults or modify the default-path resolution to also discover source files (e.g., glob("packages/*/src/**/*.py")) and append them to the returned resource paths. Locate the option definition with the mismatched help string and the function/code that computes default resource paths and implement one of the two fixes: correct the help text, or extend the default discovery to include source files before returning.tests/unit/specfact_backlog/test_auth_commands.py-74-74 (1)
74-74:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the exact contract exit code for missing subcommand.
Line 74 should assert
exit_code == 2instead of!= 0; the current check can pass unrelated failure modes and weaken the CLI contract gate.As per coding guidelines, "tests/**/*.py: Contract-first and integration tests...".Proposed test tightening
- assert result.exit_code != 0 + assert result.exit_code == 2🤖 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/specfact_backlog/test_auth_commands.py` at line 74, The test currently asserts a non-zero exit code for the missing-subcommand case; tighten it to assert the exact CLI contract by replacing the loose check on result.exit_code with an exact assertion: assert result.exit_code == 2 (use the existing result variable and its exit_code property) so the test for the missing subcommand failure is deterministic and only passes the intended contract condition.
🤖 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 @.github/workflows/docs-review.yml:
- Around line 61-66: The prompt-validation command pipeline currently swallows
the Python exit code (the line invoking "python scripts/check-prompt-commands.py
... | tee ...") so update that step to append and run exit
"${PIPESTATUS[0]:-$?}" after the pipeline to propagate failures; additionally,
harden the secondary checkout step named "Checkout paired core command sources"
(the actions/checkout@v4 invocation that sets repository: nold-ai/specfact-cli
and path: specfact-cli) by adding persist-credentials: false to avoid persisting
tokens in git config for later steps.
In @.github/workflows/pr-orchestrator.yml:
- Around line 128-134: The checkout step for the paired core CLI (uses:
actions/checkout@v4, repository: nold-ai/specfact-cli, path: specfact-cli, ref:
dev) should be hardened: set persist-credentials: false to avoid leaking runner
token, replace the moving ref: dev with a pinned ref/commit input (e.g., a
workflow input or needs.* output such as paired_core_commit or paired_core_ref)
to ensure reproducible runs, and ensure the action is pinned to an explicit
stable ref (e.g., a specific tag or SHA) if policy requires; update the checkout
step to use those inputs and persist-credentials: false.
In `@docs/bundles/project/overview.md`:
- Line 23: Replace any examples that use “--help” as part of a command path
(e.g., "specfact project --help artifacts --repo .") with canonical executable
forms (e.g., "specfact project artifacts --repo .") and, where you want to show
help output, add separate examples that call the help flag alone (e.g.,
"specfact project artifacts --help" or "specfact project --help"). Search for
occurrences of the problematic patterns involving the "project" group and its
nested "sync" Typer (references to "specfact project --help", "specfact project
sync --help", or inline "--help" followed by subcommands) and update them to use
the real subcommand invocation (like "specfact project sync ...") and distinct
help examples, ensuring CLI examples in this file match actual bundled commands.
In `@docs/reference/commands.generated.json`:
- Around line 89-106: The generated record for the command "specfact backlog
auth" has an incorrect bare_invocation value ("executes"); update the
generator/output so that the entry for the command "specfact backlog auth" sets
"bare_invocation" to "requires-subcommand" (reflecting that the command group
errors without a subcommand), and adjust the generation logic that emits
bare_invocation so it inspects the command/group runtime behavior (the CLI
command node for specfact backlog auth) to detect whether it requires
subcommands rather than assuming "executes".
In `@openspec/changes/tester-module-cli-reliability/proposal.md`:
- Line 3: The reported issue: the enumerated bug list range " `#586` through `#592`
" in the proposal header is inconsistent with the detailed list because report
"`#589`" is omitted; update the proposal so the header range and the enumerated
list match (either remove `#589` from the range or add `#589` into the enumerated
list) and ensure the document includes the required "Specification truth ...
proposal/tasks/spec deltas vs. bundle behavior." statement for traceability and
compliance with openspec markdown requirements.
In
`@packages/specfact-backlog/src/specfact_backlog/backlog_core/commands/delta.py`:
- Around line 66-67: The YAML loader can raise on malformed config and bypass
the intended CLI diagnostics; wrap the yaml.safe_load call that assigns `loaded`
(and the call to `config_path.read_text`) in a try/except that catches
yaml.YAMLError (and other parsing/IO errors), and on error set `loaded = {}` (or
fallback to an empty dict) so the existing `if not isinstance(loaded, dict):`
path still runs and shows the user-facing guidance; keep any existing logging or
debug output but do not re-raise so the CLI contract is preserved.
In `@packages/specfact-govern/resources/prompts/specfact.05-enforce.md`:
- Around line 118-119: The guidance currently points to the non-mutating command
`specfact project --help`; replace that help pointer with the actionable
mutating command that actually regenerates/updates manifests (e.g., `specfact
project sync [<bundle-name>]` or your canonical "regenerate manifests" command)
so remediation will fix hash mismatches; update both occurrences (the `specfact
project --help` strings near the `specfact govern enforce sdd` guidance at the
two locations) and ensure the text uses the canonical supported command form per
the module CLI contract docs.
In `@packages/specfact-project/resources/prompts/shared/cli-enforcement.md`:
- Around line 114-118: The doc lists `specfact project --help` as placeholders
for multiple actions which must be replaced with concrete canonical CLI
commands; update the entries so each line uses the real subcommands/flags
instead of `--help` (e.g., replace the init/select/review/create placeholder
lines with the actual commands such as the project init command, the command to
set active plan (e.g., `specfact project set-plan --plan <name>` or the
project's canonical equivalent), the plan review command, and the SDD manifest
create command), ensure consistency with the existing `specfact code import
[<bundle-name>] --repo <path>` example (use the same bundle/plan flag names like
`--bundle`/`--plan`), and make sure each example exactly matches the implemented
CLI subcommand names used by Project/Code import functions so validation passes.
In `@packages/specfact-project/resources/prompts/specfact.02-plan.md`:
- Around line 63-68: Replace the repeated help-only placeholder "specfact
project --help" used in the plan creation/update prompt with real, executable
command templates (e.g., concrete examples for creating/updating plan artifacts)
so the prompt flow is runnable; locate every occurrence of the string "specfact
project --help" in this prompt file and replace it with specific commands that
include required flags/arguments for creating/updating plans (for example a
create command and an update command), ensure the new examples match the
generated module command overview and validate them against the module docs spec
("openspec/changes/tester-module-cli-reliability/.../spec.md") so they are
syntactically correct and executable.
In `@packages/specfact-project/resources/prompts/specfact.03-review.md`:
- Around line 36-40: Replace the placeholder uses of "specfact project --help"
with the concrete, executable SpecFact CLI commands for review/export/import
flows (e.g., the real subcommands used for project review, export, and import)
so examples are accurate and runnable; ensure each restored command matches the
bundled CLI contract, provide nearest non-interactive alternatives when an
option would be interactive, and avoid any direct writes to .specfact/ by
routing artifact updates through the SpecFact CLI commands or CLI-consumed
enrichment/answers files referenced in the prompt; update every affected section
where "specfact project --help" was inserted to the corresponding real command
names and ensure examples are syntactically correct and non-interactive.
In `@packages/specfact-project/resources/prompts/specfact.04-sdd.md`:
- Around line 52-53: Replace the no-op "specfact project --help" examples in
specfact.04-sdd.md (examples referenced around lines 52, 81, 114-116, and 150,
including Phase 3) with the actual SDD create/update CLI invocations used by the
project (e.g., the canonical "specfact sdd create <bundle|--active-plan>
<options>" and "specfact sdd update <sdd-id> <options>" forms), ensuring each
example shows real flags/args and aligns with the canonical forms required by
openspec/changes/tester-module-cli-reliability/specs/modules-docs-command-validation/spec.md
and module-command-overview/spec.md; remove the duplicated help example in Phase
3 and replace it with the appropriate create/update example or an idempotent
status command used by the SDD workflow.
In `@packages/specfact-project/resources/prompts/specfact.compare.md`:
- Around line 54-56: Replace each placeholder invocation of "specfact project
--help" in specfact.compare.md with the actual runnable compare workflow CLI
examples used by the bundled tool: use the "project compare" subcommand and
include the real flags and arguments the bundle expects (e.g., the --bundle,
--from, --to and --output style flags) so the examples are executable and
validate against the bundled command signatures; update the recovery guidance
example the same way and ensure all examples match the bundled CLI's current
parameter names and ordering.
In `@packages/specfact-project/src/specfact_project/analyzers/code_analyzer.py`:
- Around line 402-403: The calls to check_tool_in_env(self.repo_path, "semgrep",
env_info) are discarding the second tuple (currently _message), which contains
the environment-specific diagnostic; update both call sites to capture the
second return value (e.g., available, message = check_tool_in_env(...)) and
forward that message into get_plugin_status()/the user-facing status logic
instead of using the generic hardcoded text so the EnvManager probe context and
guidance are preserved (symbols to change: check_tool_in_env, get_plugin_status,
variables available/message, and the semgrep probe sites in code_analyzer.py).
In `@packages/specfact-project/src/specfact_project/project/commands.py`:
- Around line 318-324: The snapshot/export command paths still call
_fetch_backlog_graph and use graph.to_json() without guarding for None; update
the snapshot and graph-export handlers to call _require_backlog_graph(graph,
command_name="snapshot" or "graph-export") immediately after obtaining graph
from _fetch_backlog_graph and use the returned value (the non-None typed graph)
for graph.to_json() (and any other graph usage), and similarly apply the same
guard where regenerate already uses _require_backlog_graph so no code path can
dereference a None graph.
In `@packages/specfact-spec/resources/prompts/shared/cli-enforcement.md`:
- Around line 114-123: Update the stale legacy reference to the active-plan
command: replace the bare `plan select` text with the canonical bundled CLI form
`specfact plan select` (or the correct namespaced command used elsewhere, e.g.,
`specfact project plan select` if that is the project-scoped variant) so the
guidance matches the other examples like `specfact project --help`; ensure the
note and any other occurrences in this file use the same fully qualified CLI
command syntax.
In `@scripts/check-docs-commands.py`:
- Around line 169-179: The current logic silently ignores JSON decode errors and
malformed shapes when reading GENERATED_COMMANDS_PATH, which can hide root
causes; update the code around GENERATED_COMMANDS_PATH, raw and generated_paths
so it fails fast: wrap json.loads(...) in a try/except to catch
json.JSONDecodeError and re-raise a clear ValueError (or log+raise) with the
path and decode error, assert that raw is a list and raise if not, and when
iterating entries require each entry to be a dict and that entry.get("command")
is a str—if an entry is malformed raise a ValueError describing the offending
entry (rather than continue); keep using CORE_COMMAND_PREFIXES and
GENERATED_COMMANDS_PATH but return generated_paths only after validation
succeeds.
---
Outside diff comments:
In @.github/workflows/docs-review.yml:
- Around line 97-107: The "Validate prompt commands" step's pipeline (python
scripts/check-prompt-commands.py 2>&1 | tee "$PROMPT_COMMAND_LOG") can mask
failures because tee exits successfully; enable failure propagation by running
the step under pipefail (e.g., prefix with set -o pipefail or use bash -o
pipefail) or explicitly check PIPESTATUS after the pipeline and exit on non-zero
(inspect PIPESTATUS[0] from the python command). Also remove or don't apply a
PIPESTATUS-style workaround to the "Validate generated command overview" step
since it isn't a pipeline—leave it to fail normally under bash -e or ensure no
redundant PIPESTATUS handling is present.
---
Minor comments:
In `@docs/getting-started/choose-your-modules.md`:
- Line 23: The docs line contains a duplicated CLI entry: remove the duplicate
`specfact project` so the command surface reads only once; update the table cell
that currently lists "`specfact project`, `specfact project`, `specfact project
sync`" to list "`specfact project`, `specfact project sync`" (ensuring
spacing/commas match adjacent rows) so the Project command surface accurately
reflects available commands.
In `@openspec/CHANGE_ORDER.md`:
- Line 44: The CHANGE_ORDER.md table row for "backlog-core | 00 |
tester-module-cli-reliability" currently lists source bugs [specfact-cli#586],
[`#587`], [`#588`], [`#590`], [`#591`], [`#592`] but omits `#589`; update that row's source
bug list to include
[specfact-cli#589](https://github.com/nold-ai/specfact-cli/issues/589) (keeping
the same comma-separated/hyperlink format) so the referenced range correctly
reads `#586`–#592.
In `@packages/specfact-govern/resources/prompts/shared/cli-enforcement.md`:
- Around line 114-119: The prompt lists several CLI examples that incorrectly
reference `specfact project --help`; update each misleading entry so the command
shown actually performs the described action: replace the `specfact project
--help` placeholder for "Initialize project bundle" with the actual
initialization command (e.g., `specfact project init` or the project bootstrap
command used in the codebase), replace the "Set active plan" example with the
real command that sets plans (e.g., `specfact project set-plan <plan-name>`),
replace the "Review plan" and "Create SDD manifest" examples with their
respective concrete commands (e.g., `specfact project review [<bundle-name>]`
and `specfact project create-sdd [<bundle-name>]`), and ensure `specfact code
import [<bundle-name>] --repo <path>` and `specfact govern enforce sdd
[<bundle-name>]` remain accurate; use the actual CLI verbs used by the project
(referencing the command names `specfact project init`, `specfact project
set-plan`, `specfact project review`, `specfact project create-sdd`) so examples
match real behavior.
In `@scripts/check-prompt-commands.py`:
- Line 377: The help text for the paths option claims defaults include
"packages/*/src/**/*.py" but the no-argument path-selection code only returns
package resource paths, causing a mismatch; either update the help string to
accurately state the real defaults or modify the default-path resolution to also
discover source files (e.g., glob("packages/*/src/**/*.py")) and append them to
the returned resource paths. Locate the option definition with the mismatched
help string and the function/code that computes default resource paths and
implement one of the two fixes: correct the help text, or extend the default
discovery to include source files before returning.
In `@tests/unit/specfact_backlog/test_auth_commands.py`:
- Line 74: The test currently asserts a non-zero exit code for the
missing-subcommand case; tighten it to assert the exact CLI contract by
replacing the loose check on result.exit_code with an exact assertion: assert
result.exit_code == 2 (use the existing result variable and its exit_code
property) so the test for the missing subcommand failure is deterministic and
only passes the intended contract condition.
🪄 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
Run ID: ff41ea13-11da-4039-b0d5-635dedb0a195
📒 Files selected for processing (91)
.github/pull_request_template.md.github/workflows/docs-review.yml.github/workflows/pr-orchestrator.ymlREADME.mddocs/adapters/azuredevops.mddocs/adapters/github.mddocs/bundles/backlog/refinement.mddocs/bundles/project/import-migration.mddocs/bundles/project/overview.mddocs/bundles/spec/validate.mddocs/getting-started/choose-your-modules.mddocs/getting-started/installation.mddocs/guides/ai-ide-workflow.mddocs/guides/brownfield-examples.mddocs/guides/brownfield-faq-and-roi.mddocs/guides/brownfield-modernization.mddocs/guides/command-chains.mddocs/guides/cross-module-chains.mddocs/guides/daily-devops-routine.mddocs/guides/integrations-overview.mddocs/guides/openspec-journey.mddocs/guides/speckit-comparison.mddocs/guides/speckit-journey.mddocs/integrations/devops-adapter-overview.mddocs/reference/README.mddocs/reference/command-syntax-policy.mddocs/reference/commands.generated.jsondocs/reference/commands.generated.mddocs/reference/commands.mddocs/team-and-enterprise/multi-repo.mdllms.txtopenspec/CHANGE_ORDER.mdopenspec/changes/tester-module-cli-reliability/.openspec.yamlopenspec/changes/tester-module-cli-reliability/TDD_EVIDENCE.mdopenspec/changes/tester-module-cli-reliability/proposal.mdopenspec/changes/tester-module-cli-reliability/specs/backlog-delta/spec.mdopenspec/changes/tester-module-cli-reliability/specs/code-review-tool-dependencies/spec.mdopenspec/changes/tester-module-cli-reliability/specs/module-cli-error-contract/spec.mdopenspec/changes/tester-module-cli-reliability/specs/module-command-overview/spec.mdopenspec/changes/tester-module-cli-reliability/specs/modules-docs-command-validation/spec.mdopenspec/changes/tester-module-cli-reliability/tasks.mdpackages/specfact-backlog/module-package.yamlpackages/specfact-backlog/resources/prompts/shared/cli-enforcement.mdpackages/specfact-backlog/resources/prompts/specfact.backlog-refine.mdpackages/specfact-backlog/resources/prompts/specfact.sync-backlog.mdpackages/specfact-backlog/src/specfact_backlog/backlog/auth_commands.pypackages/specfact-backlog/src/specfact_backlog/backlog/commands.pypackages/specfact-backlog/src/specfact_backlog/backlog_core/commands/delta.pypackages/specfact-codebase/module-package.yamlpackages/specfact-codebase/resources/prompts/shared/cli-enforcement.mdpackages/specfact-codebase/src/specfact_codebase/import_cmd/commands.pypackages/specfact-codebase/src/specfact_codebase/repro/commands.pypackages/specfact-codebase/src/specfact_codebase/validate/commands.pypackages/specfact-govern/module-package.yamlpackages/specfact-govern/resources/prompts/shared/cli-enforcement.mdpackages/specfact-govern/resources/prompts/specfact.05-enforce.mdpackages/specfact-govern/src/specfact_govern/enforce/commands.pypackages/specfact-project/module-package.yamlpackages/specfact-project/resources/prompts/shared/cli-enforcement.mdpackages/specfact-project/resources/prompts/specfact.02-plan.mdpackages/specfact-project/resources/prompts/specfact.03-review.mdpackages/specfact-project/resources/prompts/specfact.04-sdd.mdpackages/specfact-project/resources/prompts/specfact.06-sync.mdpackages/specfact-project/resources/prompts/specfact.compare.mdpackages/specfact-project/resources/templates/github-action.yml.j2packages/specfact-project/src/specfact_project/analyzers/code_analyzer.pypackages/specfact-project/src/specfact_project/import_cmd/commands.pypackages/specfact-project/src/specfact_project/plan/commands.pypackages/specfact-project/src/specfact_project/project/commands.pypackages/specfact-project/src/specfact_project/sync/commands.pypackages/specfact-project/src/specfact_project/sync_runtime/sync_perform_operation_impl.pypackages/specfact-spec/module-package.yamlpackages/specfact-spec/resources/prompts/shared/cli-enforcement.mdpackages/specfact-spec/src/specfact_spec/generate/commands.pypackages/specfact-spec/src/specfact_spec/sdd/commands.pypyproject.tomlscripts/check-command-contract.pyscripts/check-docs-commands.pyscripts/check-prompt-commands.pyscripts/generate-command-overview.pyscripts/pre-commit-quality-checks.shscripts/pre_commit_code_review.pytests/e2e/specfact_project/test_help_smoke.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/specfact_backlog/test_auth_commands.pytests/unit/specfact_backlog/test_delta_command_contract.pytests/unit/specfact_codebase/test_import_command_contract.pytests/unit/specfact_project/test_regenerate_command_contract.pytests/unit/test_check_prompt_commands_script.pytests/unit/test_global_cli_error_contract.pytests/unit/workflows/test_pr_orchestrator_signing.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
docs/**/*.md
⚙️ CodeRabbit configuration file
docs/**/*.md: User-facing and cross-site accuracy: Jekyll front matter, links per documentation-url-contract,
CLI examples matching bundled commands.
Files:
docs/bundles/spec/validate.mddocs/reference/README.mddocs/guides/brownfield-faq-and-roi.mddocs/guides/ai-ide-workflow.mddocs/team-and-enterprise/multi-repo.mddocs/guides/daily-devops-routine.mddocs/guides/integrations-overview.mddocs/guides/brownfield-examples.mddocs/guides/brownfield-modernization.mddocs/reference/command-syntax-policy.mddocs/guides/speckit-journey.mddocs/getting-started/choose-your-modules.mddocs/adapters/github.mddocs/reference/commands.mddocs/guides/cross-module-chains.mddocs/getting-started/installation.mddocs/bundles/project/overview.mddocs/reference/commands.generated.mddocs/adapters/azuredevops.mddocs/guides/command-chains.mddocs/bundles/backlog/refinement.mddocs/guides/speckit-comparison.mddocs/bundles/project/import-migration.mddocs/guides/openspec-journey.mddocs/integrations/devops-adapter-overview.md
packages/**/module-package.yaml
⚙️ CodeRabbit configuration file
packages/**/module-package.yaml: Validate metadata: name, version, commands, dependencies, and parity with packaged src.
Call out semver and signing implications when manifests or payloads change.
Files:
packages/specfact-backlog/module-package.yamlpackages/specfact-spec/module-package.yamlpackages/specfact-govern/module-package.yamlpackages/specfact-codebase/module-package.yamlpackages/specfact-project/module-package.yaml
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Files:
tests/unit/specfact_codebase/test_import_command_contract.pyscripts/pre_commit_code_review.pypackages/specfact-project/src/specfact_project/sync_runtime/sync_perform_operation_impl.pypackages/specfact-govern/src/specfact_govern/enforce/commands.pytests/unit/specfact_project/test_regenerate_command_contract.pypackages/specfact-codebase/src/specfact_codebase/repro/commands.pypackages/specfact-spec/src/specfact_spec/sdd/commands.pytests/unit/specfact_backlog/test_delta_command_contract.pytests/unit/test_global_cli_error_contract.pypackages/specfact-project/src/specfact_project/sync/commands.pypackages/specfact-codebase/src/specfact_codebase/validate/commands.pypackages/specfact-backlog/src/specfact_backlog/backlog/auth_commands.pytests/unit/workflows/test_pr_orchestrator_signing.pytests/unit/specfact_backlog/test_auth_commands.pytests/e2e/specfact_project/test_help_smoke.pypackages/specfact-spec/src/specfact_spec/generate/commands.pytests/unit/scripts/test_pre_commit_code_review.pypackages/specfact-project/src/specfact_project/project/commands.pytests/unit/test_check_prompt_commands_script.pypackages/specfact-backlog/src/specfact_backlog/backlog_core/commands/delta.pypackages/specfact-project/src/specfact_project/import_cmd/commands.pyscripts/check-docs-commands.pypackages/specfact-project/src/specfact_project/plan/commands.pyscripts/check-command-contract.pypackages/specfact-codebase/src/specfact_codebase/import_cmd/commands.pyscripts/generate-command-overview.pypackages/specfact-project/src/specfact_project/analyzers/code_analyzer.pypackages/specfact-backlog/src/specfact_backlog/backlog/commands.pyscripts/check-prompt-commands.py
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Contract-first and integration tests: migration suites, bundle validation, and flakiness.
Ensure changes to adapters or bridges have targeted coverage.
Files:
tests/unit/specfact_codebase/test_import_command_contract.pytests/unit/specfact_project/test_regenerate_command_contract.pytests/unit/specfact_backlog/test_delta_command_contract.pytests/unit/test_global_cli_error_contract.pytests/unit/workflows/test_pr_orchestrator_signing.pytests/unit/specfact_backlog/test_auth_commands.pytests/e2e/specfact_project/test_help_smoke.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/test_check_prompt_commands_script.py
openspec/**/*.md
⚙️ CodeRabbit configuration file
openspec/**/*.md: Specification truth: proposal/tasks/spec deltas vs. bundle behavior, CHANGE_ORDER, and
drift vs. shipped modules or docs.
Files:
openspec/changes/tester-module-cli-reliability/specs/backlog-delta/spec.mdopenspec/changes/tester-module-cli-reliability/specs/module-cli-error-contract/spec.mdopenspec/changes/tester-module-cli-reliability/specs/module-command-overview/spec.mdopenspec/changes/tester-module-cli-reliability/specs/code-review-tool-dependencies/spec.mdopenspec/changes/tester-module-cli-reliability/specs/modules-docs-command-validation/spec.mdopenspec/CHANGE_ORDER.mdopenspec/changes/tester-module-cli-reliability/proposal.mdopenspec/changes/tester-module-cli-reliability/tasks.mdopenspec/changes/tester-module-cli-reliability/TDD_EVIDENCE.md
scripts/**/*.py
⚙️ CodeRabbit configuration file
scripts/**/*.py: Deterministic tooling: signing, publishing, docs generation; subprocess and path safety.
Files:
scripts/pre_commit_code_review.pyscripts/check-docs-commands.pyscripts/check-command-contract.pyscripts/generate-command-overview.pyscripts/check-prompt-commands.py
packages/**/src/**/*.py
⚙️ CodeRabbit configuration file
packages/**/src/**/*.py: Focus on adapter and bridge patterns: imports from specfact_cli (models, runtime, validators),
Typer/Rich command surfaces, and clear boundaries so core upgrades do not silently break bundles.
Flag breaking assumptions about registry loading, lazy imports, and environment/mode behavior.
Files:
packages/specfact-project/src/specfact_project/sync_runtime/sync_perform_operation_impl.pypackages/specfact-govern/src/specfact_govern/enforce/commands.pypackages/specfact-codebase/src/specfact_codebase/repro/commands.pypackages/specfact-spec/src/specfact_spec/sdd/commands.pypackages/specfact-project/src/specfact_project/sync/commands.pypackages/specfact-codebase/src/specfact_codebase/validate/commands.pypackages/specfact-backlog/src/specfact_backlog/backlog/auth_commands.pypackages/specfact-spec/src/specfact_spec/generate/commands.pypackages/specfact-project/src/specfact_project/project/commands.pypackages/specfact-backlog/src/specfact_backlog/backlog_core/commands/delta.pypackages/specfact-project/src/specfact_project/import_cmd/commands.pypackages/specfact-project/src/specfact_project/plan/commands.pypackages/specfact-codebase/src/specfact_codebase/import_cmd/commands.pypackages/specfact-project/src/specfact_project/analyzers/code_analyzer.pypackages/specfact-backlog/src/specfact_backlog/backlog/commands.py
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: CI: secrets, hatch/verify-modules-signature gates, contract-test alignment, action versions.
Files:
.github/workflows/docs-review.yml.github/workflows/pr-orchestrator.yml
🪛 LanguageTool
llms.txt
[uncategorized] ~22-~22: The official name of this software platform is spelled with a capital “H”.
Context: ...og| -; args: - | azure-devops, clear, github, status | | |specfact backlog auth a...
(GITHUB)
[uncategorized] ~25-~25: The official name of this software platform is spelled with a capital “H”.
Context: ...cklog| --provider; args: - | - | | |specfact backlog auth github| nold-ai/specfact-backlog |specfact...
(GITHUB)
docs/reference/commands.generated.md
[uncategorized] ~18-~18: The official name of this software platform is spelled with a capital “H”.
Context: ...og| -; args: - | azure-devops, clear, github, status | | |specfact backlog auth a...
(GITHUB)
[uncategorized] ~21-~21: The official name of this software platform is spelled with a capital “H”.
Context: ...cklog| --provider; args: - | - | | |specfact backlog auth github| nold-ai/specfact-backlog |specfact...
(GITHUB)
🪛 Shellcheck (0.11.0)
scripts/pre-commit-quality-checks.sh
[warning] 81-81: This pattern always overrides a later one on line 81.
(SC2221)
[warning] 81-81: This pattern never matches because of a previous pattern on line 81.
(SC2222)
🪛 zizmor (1.25.2)
.github/workflows/docs-review.yml
[warning] 61-66: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 62-62: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
.github/workflows/pr-orchestrator.yml
[warning] 128-134: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 130-130: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🔀 Multi-repo context nold-ai/specfact-cli
[::nold-ai/specfact-cli::] .github/workflows and CI hooks now expect/produce:
- .github/workflows/* and scripts reference SPECFACT_CLI_REPO and SPECFACT_MODULES_ROOTS (e.g. scripts/pre_commit_code_review.py lines ~127–130 set/extend SPECFACT_MODULES_ROOTS). Found: scripts/pre_commit_code_review.py:127,130.
- pyproject.toml defines contract-test-contracts hatch entry (pyproject.toml:292,316) and repo-wide guidance (.github/pull_request_template.md) updated to require
hatch run contract-test-contracts. - scripts/setup-git-hooks.sh echoes
hatch run contract-test-contracts(scripts/setup-git-hooks.sh:78).
[::nold-ai/specfact-cli::] Generated-command & contract-check integration:
- Tests and scripts expect generated command artifacts / canonical command names (
project sync bridge) — seen in tests and docs: tests/unit/validation/test_command_audit.py (expects "project sync bridge"), many docs updated tospecfact project sync bridge(multiple docs/* entries). - Repository contains scripts and tests wiring that modules PR depends on: scripts/ (exists) and tests referencing SPECFACT_MODULES_ROOTS and generated command validation (tests/unit/scripts/test_pre_commit_code_review.py asserts SPECFACT_MODULES_ROOTS value; tests/e2e and unit tests reference
specfact_project.project.commandshelp strings).
Assessment / relevance to reviewed PR:
- The modules PR’s workflow changes (checking out nold-ai/specfact-cli, exporting SPECFACT_CLI_REPO, switching to hatch-based contract checks, running
contract-test-contractsand new command-overview/contract checks) align with existing hooks and CI entries in this repo (pyproject.toml, scripts, and tests). Integration points to verify during review:- SPECFACT_MODULES_ROOTS usage and interactions between core (this repo) and modules repo (the PR’s added checkout / editable install behavior). [::nold-ai/specfact-cli::scripts/pre_commit_code_review.py:127–130]
- That
hatch run contract-test-contractstarget exists and behaves as the modules PR expects (pyproject.toml entries). [::nold-ai/specfact-cli::pyproject.toml:292,316] - Generated command artifacts / command-contract check scripts are consumed by workflows/tests (tests reference canonical
project sync bridge). Confirm compatibility of modules’ generated artifacts with core’s script mounts/imports. [::nold-ai/specfact-cli::tests/unit/validation/test_command_audit.py:35 and many docs references]
🔇 Additional comments (56)
README.md (1)
5-9: LGTM!openspec/changes/tester-module-cli-reliability/.openspec.yaml (1)
1-2: LGTM!docs/bundles/spec/validate.md (1)
27-27: LGTM!packages/specfact-backlog/resources/prompts/specfact.backlog-refine.md (1)
527-527: LGTM!Also applies to: 532-532
docs/reference/README.md (1)
37-37: LGTM!Also applies to: 42-42
docs/guides/brownfield-faq-and-roi.md (1)
21-21: LGTM!docs/guides/ai-ide-workflow.md (1)
46-46: LGTM!packages/specfact-backlog/src/specfact_backlog/backlog/commands.py (1)
337-337: LGTM!Also applies to: 351-351, 363-363, 378-378, 393-393, 2844-2844, 3312-3312, 4101-4101
packages/specfact-backlog/module-package.yaml (1)
2-2: LGTM!Also applies to: 30-31
packages/specfact-spec/module-package.yaml (1)
2-2: LGTM!Also applies to: 24-25
openspec/changes/tester-module-cli-reliability/tasks.md (1)
1-40: LGTM!llms.txt (1)
1-104: LGTM!docs/bundles/project/import-migration.md (1)
89-89: LGTM!Also applies to: 92-92, 141-141, 149-149, 194-194
tests/unit/specfact_codebase/test_import_command_contract.py (1)
8-15: LGTM!packages/specfact-govern/module-package.yaml (1)
2-2: LGTM!Also applies to: 22-23
openspec/changes/tester-module-cli-reliability/specs/backlog-delta/spec.md (1)
3-19: LGTM!scripts/pre_commit_code_review.py (1)
152-159: LGTM!openspec/changes/tester-module-cli-reliability/specs/module-cli-error-contract/spec.md (1)
3-37: LGTM!packages/specfact-codebase/module-package.yaml (1)
2-2: LGTM!Also applies to: 27-28
tests/unit/scripts/test_pre_commit_code_review.py (1)
9-9: LGTM!Also applies to: 358-379
.github/pull_request_template.md (1)
38-40: LGTM!openspec/changes/tester-module-cli-reliability/specs/module-command-overview/spec.md (1)
1-27: LGTM!docs/team-and-enterprise/multi-repo.md (1)
33-33: LGTM!docs/guides/daily-devops-routine.md (1)
39-39: LGTM!packages/specfact-backlog/resources/prompts/shared/cli-enforcement.md (1)
114-118: LGTM!Also applies to: 120-120
packages/specfact-codebase/resources/prompts/shared/cli-enforcement.md (1)
114-118: LGTM!Also applies to: 120-120
packages/specfact-backlog/resources/prompts/specfact.sync-backlog.md (1)
159-159: LGTM!Also applies to: 167-167, 179-179, 187-187, 235-235, 242-242, 313-313, 319-319, 390-390, 397-397, 416-416, 423-423
scripts/generate-command-overview.py (1)
20-24: LGTM!Also applies to: 25-33, 35-65, 67-73, 75-87, 89-102, 104-111, 113-143, 145-175, 177-197, 199-220, 222-238
openspec/changes/tester-module-cli-reliability/specs/code-review-tool-dependencies/spec.md (1)
1-20: LGTM!docs/guides/integrations-overview.md (1)
48-48: LGTM!Also applies to: 63-63, 66-66
docs/guides/brownfield-examples.md (1)
19-19: LGTM!Also applies to: 33-33
packages/specfact-project/resources/templates/github-action.yml.j2 (1)
27-31: LGTM!scripts/check-command-contract.py (1)
1-234: LGTM!packages/specfact-codebase/src/specfact_codebase/import_cmd/commands.py (1)
15-59: LGTM!Also applies to: 78-78, 150-151, 168-169
docs/guides/speckit-journey.md (1)
59-60: LGTM!Also applies to: 65-66
docs/getting-started/choose-your-modules.md (1)
253-253: LGTM!packages/specfact-project/src/specfact_project/sync_runtime/sync_perform_operation_impl.py (1)
37-37: LGTM!Also applies to: 56-57, 133-133
packages/specfact-codebase/src/specfact_codebase/validate/commands.py (1)
146-150: LGTM!Also applies to: 214-221
docs/guides/speckit-comparison.md (1)
219-223: LGTM!Also applies to: 228-232, 303-304, 310-311, 321-323
packages/specfact-spec/src/specfact_spec/generate/commands.py (1)
1576-1576: LGTM!Also applies to: 1797-1798, 1813-1816, 1841-1841
docs/guides/cross-module-chains.md (1)
30-30: LGTM!Also applies to: 38-38
docs/reference/commands.generated.md (1)
1-101: LGTM!docs/adapters/azuredevops.md (1)
70-70: LGTM!Also applies to: 440-440, 450-450, 460-460, 472-472, 484-484, 496-496
packages/specfact-codebase/src/specfact_codebase/repro/commands.py (1)
10-10: LGTM!Also applies to: 78-80, 133-133
packages/specfact-project/module-package.yaml (1)
2-2: LGTM!Also applies to: 30-31
tests/unit/workflows/test_pr_orchestrator_signing.py (1)
3-3: LGTM!Also applies to: 40-53
tests/unit/test_check_prompt_commands_script.py (1)
166-207: LGTM!Also applies to: 274-283, 288-289, 298-299, 323-324, 332-333
packages/specfact-project/resources/prompts/specfact.06-sync.md (1)
73-80: LGTM!Also applies to: 111-112, 144-146, 180-186
packages/specfact-spec/src/specfact_spec/sdd/commands.py (1)
260-260: LGTM!docs/guides/command-chains.md (1)
29-29: LGTM!Also applies to: 43-43
tests/unit/specfact_backlog/test_delta_command_contract.py (1)
12-58: LGTM!tests/unit/test_global_cli_error_contract.py (1)
12-46: LGTM!tests/e2e/specfact_project/test_help_smoke.py (1)
23-29: LGTM!packages/specfact-project/src/specfact_project/import_cmd/commands.py (1)
1673-1673: LGTM!Also applies to: 1721-1721, 1952-1952, 2026-2026, 2051-2051, 2093-2095, 2494-2499
packages/specfact-project/src/specfact_project/plan/commands.py (1)
2528-2528: LGTM!Also applies to: 2558-2558, 2736-2736, 3612-3612, 3637-3637
docs/getting-started/installation.md (1)
286-287: LGTM!Also applies to: 291-291, 300-300, 303-303, 306-306
Comment on lines
+61
to
+66
| - name: Checkout paired core command sources | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: nold-ai/specfact-cli | ||
| path: specfact-cli | ||
| ref: ${{ (github.ref == 'refs/heads/main' || github.head_ref == 'main') && 'main' || 'dev' }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify action refs and credential persistence in the changed workflow.
rg -n 'uses:\s*actions/checkout@' .github/workflows/docs-review.yml
rg -n 'persist-credentials' .github/workflows/docs-review.ymlRepository: nold-ai/specfact-cli-modules
Length of output: 146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/docs-review.yml"
# Show the exact checkout step section (incl. line numbers around 61-66)
nl -ba "$FILE" | sed -n '45,130p'
# Targeted checks for the specific constructs mentioned in the review
echo "---- actions/checkout@ occurrences ----"
rg -n "uses:\s*actions/checkout@" "$FILE" || true
echo "---- persist-credentials occurrences ----"
rg -n "persist-credentials" "$FILE" || true
echo "---- prompt/overview run blocks around 97-107 ----"
nl -ba "$FILE" | sed -n '85,120p'Repository: nold-ai/specfact-cli-modules
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/docs-review.yml"
# Show the exact checkout step section (incl. line numbers around ~61-66)
echo "---- lines 50-80 ----"
cat -n "$FILE" | sed -n '50,80p'
# Targeted checks for the constructs mentioned in the review
echo "---- actions/checkout@ occurrences ----"
rg -n "uses:\s*actions/checkout@" "$FILE" || true
echo "---- persist-credentials occurrences ----"
rg -n "persist-credentials" "$FILE" || true
# Prompt/overview run blocks around ~97-107
echo "---- lines 80-130 ----"
cat -n "$FILE" | sed -n '80,130p'Repository: nold-ai/specfact-cli-modules
Length of output: 3082
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for pinned checkout usage elsewhere
rg -n --hidden "uses:\s*actions/checkout@([a-f0-9]{7,40})" .github/workflows || true
rg -n --hidden "uses:\s*actions/checkout@" .github/workflows || true
# Check for persist-credentials usage elsewhere
rg -n --hidden "persist-credentials:\s*false" .github/workflows || trueRepository: nold-ai/specfact-cli-modules
Length of output: 963
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- persist-credentials usage ----"
rg -n "persist-credentials" .github/workflows || true
echo "---- checkout pinned to SHA (approx) ----"
rg -n --hidden "uses:\s*actions/checkout@([a-f0-9]{7,40})" .github/workflows || trueRepository: nold-ai/specfact-cli-modules
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "actions/checkout@|persist-credentials" .github . || true
rg -n "pin(ned)? to a SHA|pin to a SHA|recommended.*pin|security.*checkout" .github . || trueRepository: nold-ai/specfact-cli-modules
Length of output: 1525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "actions/checkout|persist-credentials|pin(ned)?\\s+to\\s+(a\\s+)?SHA|pin\\s+to\\s+a\\s+SHA|action versions|workflow.*security" .github docs . || trueRepository: nold-ai/specfact-cli-modules
Length of output: 1504
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/docs-review.yml"
echo "---- file top (1-55) ----"
cat -n "$FILE" | sed -n '1,55p'Repository: nold-ai/specfact-cli-modules
Length of output: 2256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/docs-review.yml"
rg -n "pipefail|set -o pipefail|set -e|defaults:|shell:" "$FILE" || trueRepository: nold-ai/specfact-cli-modules
Length of output: 54
🌐 Web query:
GitHub Actions default bash run step options -e and -o pipefail when using run: |?
💡 Result:
In GitHub Actions, the shell options applied to a run step depend on whether you explicitly specify shell: bash or rely on the default behavior [1]. When you use run: | without specifying a shell, GitHub Actions attempts to execute the script using a default shell [2][3]. On non-Windows platforms, this is typically bash if it is available, or sh as a fallback [2][4]. When no shell is explicitly defined, these default invocations typically include set -e (fail-fast on errors) but do not include -o pipefail [5][1][3]. When you explicitly specify shell: bash in your workflow step, GitHub Actions uses the template bash --noprofile --norc -eo pipefail {0} [2][4]. This explicitly enables both -e (exit immediately if a command exits with a non-zero status) and -o pipefail (ensure the exit status of a pipeline is the value of the last command to exit with a non-zero status, or zero if no command exited with a non-zero status) [1][4]. To ensure consistent fail-fast behavior across complex pipelines that use pipes, it is recommended to explicitly set the shell: yaml jobs: example: runs-on: ubuntu-latest steps: - name: Run with pipefail shell: bash run: | # Now -eo pipefail is guaranteed command_that_might_fail | grep "pattern" If you need even stricter control, you can define a custom shell string in your workflow or job defaults, such as shell: bash -eo pipefail {0} [2][6].
Citations:
- 1: https://stackoverflow.com/questions/75419587/does-a-github-action-step-use-set-e-semantics-by-default
- 2: https://github.com/actions/runner/blob/main/docs/adrs/0277-run-action-shell-options.md
- 3: Default non-Windows bash is not invoked as documented actions/runner#353
- 4: https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions
- 5: Default
bashshell doesn't seem to run with-o pipefailactions/runner-images#4459 - 6: https://stackoverflow.com/questions/70085548/running-a-login-shell-in-github-actions
Fix masked CI failures in prompt validation and harden the secondary checkout
.github/workflows/docs-review.yml(lines 97-102):python scripts/check-prompt-commands.py ... | tee ...doesn’t propagate the Python exit code, so prompt validation failures can be ignored—addexit "${PIPESTATUS[0]:-$?}"like the earlier docs steps..github/workflows/docs-review.yml(lines 61-66): the secondary checkout ofnold-ai/specfact-clishould setpersist-credentials: falseto avoid persisting the token in git config for subsequent steps.- Drop the “pin
actions/checkoutto a SHA” ask—this repo’s workflows/templates consistently useactions/checkout@v4.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 61-66: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 62-62: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 61 - 66, The
prompt-validation command pipeline currently swallows the Python exit code (the
line invoking "python scripts/check-prompt-commands.py ... | tee ...") so update
that step to append and run exit "${PIPESTATUS[0]:-$?}" after the pipeline to
propagate failures; additionally, harden the secondary checkout step named
"Checkout paired core command sources" (the actions/checkout@v4 invocation that
sets repository: nold-ai/specfact-cli and path: specfact-cli) by adding
persist-credentials: false to avoid persisting tokens in git config for later
steps.
Comment on lines
+128
to
+134
| - name: Checkout paired core CLI | ||
| if: needs.changes.outputs.skip_tests_dev_to_main != 'true' | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: nold-ai/specfact-cli | ||
| path: specfact-cli | ||
| ref: dev |
Contributor
There was a problem hiding this comment.
Harden external checkout step to avoid credential leakage and drifting inputs
The paired-core checkout currently persists credentials and tracks a moving dev ref. For a gate workflow, this increases credential exposure and makes results non-reproducible across runs.
Suggested hardening
- name: Checkout paired core CLI
if: needs.changes.outputs.skip_tests_dev_to_main != 'true'
- uses: actions/checkout@v4
+ uses: actions/checkout@<pinned-commit-sha>
with:
repository: nold-ai/specfact-cli
path: specfact-cli
- ref: dev
+ ref: <trusted-commit-or-tag>
+ persist-credentials: falseAs per coding guidelines, .github/workflows/**: CI: secrets, hatch/verify-modules-signature gates, contract-test alignment, action versions.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 128-134: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 130-130: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 128 - 134, The checkout
step for the paired core CLI (uses: actions/checkout@v4, repository:
nold-ai/specfact-cli, path: specfact-cli, ref: dev) should be hardened: set
persist-credentials: false to avoid leaking runner token, replace the moving
ref: dev with a pinned ref/commit input (e.g., a workflow input or needs.*
output such as paired_core_commit or paired_core_ref) to ensure reproducible
runs, and ensure the action is pinned to an explicit stable ref (e.g., a
specific tag or SHA) if policy requires; update the checkout step to use those
inputs and persist-credentials: false.
| ## Command families | ||
|
|
||
| The project lifecycle surface loads from this bundle across **`specfact project`**, **`specfact plan`**, top-level **`specfact sync`**, and **`specfact migrate`** (see each group’s `--help` after install). The `project` group also nests a `sync` Typer; prefer the **top-level** `specfact sync …` entry when documented in the command reference. | ||
| The project lifecycle surface loads from this bundle across **`specfact project`**, **`specfact project`**, top-level **`specfact project sync`**, and **`specfact project --help`** (see each group’s `--help` after install). The `project` group also nests a `sync` Typer; prefer the **top-level** `specfact project sync …` entry when documented in the command reference. |
Contributor
There was a problem hiding this comment.
Invalid command surface examples (--help used as command path)
This section treats specfact project --help as a command group and shows specfact project --help artifacts --repo . (invalid invocation). Please replace these with canonical executable forms (e.g., specfact project artifacts --repo ., plus separate ... --help examples where needed).
As per coding guidelines, docs/**/*.md: “CLI examples matching bundled commands.”
Also applies to: 69-69, 91-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/bundles/project/overview.md` at line 23, Replace any examples that use
“--help” as part of a command path (e.g., "specfact project --help artifacts
--repo .") with canonical executable forms (e.g., "specfact project artifacts
--repo .") and, where you want to show help output, add separate examples that
call the help flag alone (e.g., "specfact project artifacts --help" or "specfact
project --help"). Search for occurrences of the problematic patterns involving
the "project" group and its nested "sync" Typer (references to "specfact project
--help", "specfact project sync --help", or inline "--help" followed by
subcommands) and update them to use the real subcommand invocation (like
"specfact project sync ...") and distinct help examples, ensuring CLI examples
in this file match actual bundled commands.
Comment on lines
+89
to
+106
| "arguments": [], | ||
| "bare_invocation": "executes", | ||
| "command": "specfact backlog auth", | ||
| "deprecated": false, | ||
| "hidden": false, | ||
| "install_prerequisite": "specfact module install nold-ai/specfact-backlog", | ||
| "options": [], | ||
| "owner_package": "nold-ai/specfact-backlog", | ||
| "owner_repo": "nold-ai/specfact-cli-modules", | ||
| "short_help": "", | ||
| "source": "specfact_backlog.backlog.commands:app", | ||
| "subcommands": [ | ||
| "azure-devops", | ||
| "clear", | ||
| "github", | ||
| "status" | ||
| ] | ||
| }, |
Contributor
There was a problem hiding this comment.
bare_invocation for specfact backlog auth is inconsistent with runtime behavior.
This record says "executes", but the command group now errors when invoked without a subcommand (Exit(2)), so this should be "requires-subcommand" to keep generated contract data truthful.
As per coding guidelines, “Generate deterministic module command overview artifacts from the actual module command tree … [and make] generated command overview the canonical source for docs validation.”
🤖 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 89 - 106, The generated
record for the command "specfact backlog auth" has an incorrect bare_invocation
value ("executes"); update the generator/output so that the entry for the
command "specfact backlog auth" sets "bare_invocation" to "requires-subcommand"
(reflecting that the command group errors without a subcommand), and adjust the
generation logic that emits bare_invocation so it inspects the command/group
runtime behavior (the CLI command node for specfact backlog auth) to detect
whether it requires subcommands rather than assuming "executes".
| @@ -0,0 +1,43 @@ | |||
| ## Why | |||
|
|
|||
| Primary tester reports filed in `nold-ai/specfact-cli#586` through `#592` expose module-owned CLI contract failures: project regeneration crashes on missing/null bundle data, sync bridge help still advertises removed flat commands, code import docs/help allow invalid option ordering, semgrep diagnostics miss the active uv environment, and backlog command groups report missing input without actionable help. | |||
Contributor
There was a problem hiding this comment.
Source bug range and enumerated list are inconsistent.
Line 3 says reports span #586 through #592, but Line 39 omits #589. Please align the list/range so traceability is complete and unambiguous.
As per coding guidelines, openspec/**/*.md requires “Specification truth ... proposal/tasks/spec deltas vs. bundle behavior.”
Also applies to: 39-39
🤖 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 `@openspec/changes/tester-module-cli-reliability/proposal.md` at line 3, The
reported issue: the enumerated bug list range " `#586` through `#592` " in the
proposal header is inconsistent with the detailed list because report "`#589`" is
omitted; update the proposal so the header range and the enumerated list match
(either remove `#589` from the range or add `#589` into the enumerated list) and
ensure the document includes the required "Specification truth ...
proposal/tasks/spec deltas vs. bundle behavior." statement for traceability and
compliance with openspec markdown requirements.
Comment on lines
+54
to
56
| specfact project --help | ||
| # --bundle defaults to active plan if not specified | ||
| ``` |
Contributor
There was a problem hiding this comment.
Replace help placeholders with executable compare workflow commands.
Line 54, Line 85, and Line 118 currently use specfact project --help where the workflow requires runnable commands; Line 149 does the same for recovery guidance. This breaks prompt operability and command-validation parity.
Suggested direction
- specfact project --help
+ specfact compare --code-vs-plan --output-format markdown
- specfact project --help
+ specfact compare --bundle <bundle-name> --output-format markdown
- specfact project --help
- specfact project --help
+ specfact plan update-feature --batch-updates /tmp/feature-updates.json
+ specfact compare --bundle <bundle-name> --output-format markdown
- Create one with: specfact project --help
+ Create one with: specfact plan init --interactiveAs per coding guidelines, docs/prompt command examples must match bundled commands and stay executable (“docs/**/*.md: ... CLI examples matching bundled commands”).
Also applies to: 85-86, 118-120, 149-150
🤖 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 `@packages/specfact-project/resources/prompts/specfact.compare.md` around lines
54 - 56, Replace each placeholder invocation of "specfact project --help" in
specfact.compare.md with the actual runnable compare workflow CLI examples used
by the bundled tool: use the "project compare" subcommand and include the real
flags and arguments the bundle expects (e.g., the --bundle, --from, --to and
--output style flags) so the examples are executable and validate against the
bundled command signatures; update the recovery guidance example the same way
and ensure all examples match the bundled CLI's current parameter names and
ordering.
Comment on lines
+402
to
+403
| available, _message = check_tool_in_env(self.repo_path, "semgrep", env_info) | ||
| return available |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify semgrep probe calls and whether returned messages are discarded
rg -n "check_tool_in_env\\(self\\.repo_path, \"semgrep\", env_info\\)" -C2 \
packages/specfact-project/src/specfact_project/analyzers/code_analyzer.py
# Verify fallback reason text in plugin status remains generic
rg -n "Semgrep CLI not installed|Pattern detection enabled|reason =" -C2 \
packages/specfact-project/src/specfact_project/analyzers/code_analyzer.pyRepository: nold-ai/specfact-cli-modules
Length of output: 1422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate implementations
rg -n "def (check_tool_in_env|detect_env_manager)\\b" packages/specfact-project/src/specfact_project/analyzers/code_analyzer.py packages/specfact-project/src/specfact_project -S
# Dump the relevant function bodies (keep it small)
python3 - <<'PY'
import subprocess, re, json, os, textwrap
path="packages/specfact-project/src/specfact_project"
# Find candidate files containing these defs
cmd=["bash","-lc",f"rg -n \"def (check_tool_in_env|detect_env_manager)\\\\b\" {path} -S"]
out=subprocess.check_output(cmd, text=True)
hits=[]
for line in out.splitlines():
m=re.match(r'(.+?):(\d+):(.*)', line)
if not m:
continue
file=m.group(1); ln=int(m.group(2)); rest=m.group(3)
if "check_tool_in_env" in rest or "detect_env_manager" in rest:
hits.append((file,ln,rest.strip()))
# Limit to first few unique files
seen=set()
for file,ln,rest in hits:
if file in seen:
continue
seen.add(file)
# Print 80 lines around definition
print("\n=== FILE:",file,"DEF_LINE:",ln,"===\n")
# Use sed to avoid huge output; best-effort
subprocess.run(["bash","-lc",f"sed -n '{max(1,ln-20)},{ln+120}p' {file}"], check=False)
PY
# Search for other uses of the second return value from check_tool_in_env
rg -n "check_tool_in_env\\([^\\)]*,\\s*\"semgrep\"|check_tool_in_env\\(.*\\)" packages/specfact-project/src/specfact_project -S
# Specifically find tuple-unpacking patterns where the message is used
rg -n "check_tool_in_env\\([^\\)]*\\)\\s*\\)\\s*\\n|available,\\s*_message|available,\\s*message|,\\s*message\\s*=" packages/specfact-project/src/specfact_project -SRepository: nold-ai/specfact-cli-modules
Length of output: 54
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where these are defined anywhere under specfact-project
rg -n "def (check_tool_in_env|detect_env_manager)\\b" packages/specfact-project/src -S
# Show check_tool_in_env call sites that mention semgrep
rg -n "check_tool_in_env\\([^\\)]*,\\s*\"semgrep\"" packages/specfact-project/src -S -C2
# Find any uses of the returned message (second tuple element) from check_tool_in_env
rg -n "([a-zA-Z_][a-zA-Z0-9_]*)\\s*,\\s*([a-zA-Z_][a-zA-Z0-9_]*)\\s*=\\s*check_tool_in_env\\(" packages/specfact-project/src -S -C2
rg -n "check_tool_in_env\\([^\\)]*\\)\\s*\\)" packages/specfact-project/src -S
# Show the semgrep availability logic section around code_analyzer line ~380-520 (as referenced by the original comment)
sed -n '360,540p' packages/specfact-project/src/specfact_project/analyzers/code_analyzer.pyRepository: nold-ai/specfact-cli-modules
Length of output: 54
🏁 Script executed:
#!/bin/bash
set -u
# Find where these are defined anywhere under specfact-project (don't fail the script on no matches)
rg -n "def (check_tool_in_env|detect_env_manager)\\b" packages/specfact-project/src -S || true
# Show check_tool_in_env call sites that mention semgrep
rg -n "check_tool_in_env\\([^\\)]*,\\s*\"semgrep\"" packages/specfact-project/src -S -C2 || true
# Find any tuple-unpacking patterns where the second element is named (potentially used)
rg -n "available,\\s*_message\\s*=\\s*check_tool_in_env\\(" packages/specfact-project/src -S -C2 || true
rg -n "available,\\s*message\\s*=\\s*check_tool_in_env\\(" packages/specfact-project/src -S -C2 || true
rg -n ",\\s*_message\\s*=\\s*check_tool_in_env\\(" packages/specfact-project/src -S -C2 || true
rg -n ",\\s*message\\s*=\\s*check_tool_in_env\\(" packages/specfact-project/src -S -C2 || true
# Fallback: show the file region referenced in the original comment
sed -n '360,540p' packages/specfact-project/src/specfact_project/analyzers/code_analyzer.py || trueRepository: nold-ai/specfact-cli-modules
Length of output: 10331
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate implementations
rg -n "def (check_tool_in_env|detect_env_manager|build_tool_command)\\b" -S .
# Show the full env_manager file (or at least the relevant regions)
envfile=$(rg -l "def (check_tool_in_env|detect_env_manager)\\b" -S . | head -n 1)
echo "ENV_FILE=$envfile"
sed -n '1,240p' "$envfile"
sed -n '240,520p' "$envfile"
# Find semgrep-specific call patterns (if any)
rg -n "semgrep" -S .Repository: nold-ai/specfact-cli
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect tests that validate Semgrep plugin status/reason
sed -n '740,820p' tests/integration/analyzers/test_code_analyzer_integration.py || true
rg -n "Semgrep Pattern Detection|reason\"\\]|reason =|Semgrep CLI not installed" tests -S || true
# Also inspect any unit tests around get_plugin_status / semgrep availability
rg -n "get_plugin_status\\(|_check_semgrep_available|Semgrep CLI not installed" packages/specfact-project/src/specfact_project tests -S || trueRepository: nold-ai/specfact-cli-modules
Length of output: 1362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "Semgrep CLI not installed" -S packages/specfact-project/src/specfact_project tests || true
rg -n "\"Semgrep Pattern Detection\"|Semgrep Pattern Detection" -S packages/specfact-project/src/specfact_project tests || true
# Show the plugin status structure usage in import command
sed -n '430,520p' packages/specfact-project/src/specfact_project/import_cmd/commands.py || true
# Locate the tests that cover get_plugin_status reason text (if any)
rg -n "get_plugin_status\\(" -S tests packages/specfact-project/src/specfact_project || true
rg -n "plugin_status.*Semgrep|Semgrep.*reason" -S tests packages/specfact-project/src/specfact_project || trueRepository: nold-ai/specfact-cli-modules
Length of output: 4591
Preserve Semgrep probe diagnostics (_message) instead of discarding them
In packages/specfact-project/src/specfact_project/analyzers/code_analyzer.py, both call sites discard the second tuple value from check_tool_in_env(..., "semgrep", env_info) (lines 402-403 and 493-495). check_tool_in_env returns a context string tied to the detected EnvManager (and/or why it can’t find Semgrep in the probed environment). Dropping it leaves get_plugin_status() using hardcoded generic text (e.g., Semgrep CLI not installed (install: pip install semgrep)), so the probed manager context and probe-provided guidance can’t reach user-facing “tool unavailable” diagnostics.
🤖 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 `@packages/specfact-project/src/specfact_project/analyzers/code_analyzer.py`
around lines 402 - 403, The calls to check_tool_in_env(self.repo_path,
"semgrep", env_info) are discarding the second tuple (currently _message), which
contains the environment-specific diagnostic; update both call sites to capture
the second return value (e.g., available, message = check_tool_in_env(...)) and
forward that message into get_plugin_status()/the user-facing status logic
instead of using the generic hardcoded text so the EnvManager probe context and
guidance are preserved (symbols to change: check_tool_in_env, get_plugin_status,
variables available/message, and the semgrep probe sites in code_analyzer.py).
Comment on lines
+318
to
+324
| def _require_backlog_graph(graph: Any, *, command_name: str) -> Any: | ||
| """Fail with a typed diagnostic when backlog graph data is unavailable.""" | ||
| if graph is None or not hasattr(graph, "items"): | ||
| print_error(f"Backlog graph data unavailable for `specfact project {command_name}`.") | ||
| print_info("Run `specfact backlog analyze-deps` or `specfact backlog graph-export` to inspect backlog data.") | ||
| raise typer.Exit(1) | ||
| return graph |
Contributor
There was a problem hiding this comment.
Apply backlog-graph guard consistently to snapshot/export paths.
You added a good typed guard for regenerate, but the same source (_fetch_backlog_graph) is still used unguarded in snapshot (it calls graph.to_json()), and _fetch_backlog_graph currently returns None. That leaves a reachable NoneType crash in another command path.
Suggested fix
@@
def snapshot(
@@
- graph = _fetch_backlog_graph(adapter=adapter, project_id=project_id, template=template)
+ graph = _fetch_backlog_graph(adapter=adapter, project_id=project_id, template=template)
+ graph = _require_backlog_graph(graph, command_name="snapshot")
output.parent.mkdir(parents=True, exist_ok=True)
output.write_text(graph.to_json(), encoding="utf-8")As per coding guidelines, module adapter/bridge command surfaces should fail with clear typed diagnostics rather than leaking runtime crashes.
Also applies to: 646-646
🤖 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 `@packages/specfact-project/src/specfact_project/project/commands.py` around
lines 318 - 324, The snapshot/export command paths still call
_fetch_backlog_graph and use graph.to_json() without guarding for None; update
the snapshot and graph-export handlers to call _require_backlog_graph(graph,
command_name="snapshot" or "graph-export") immediately after obtaining graph
from _fetch_backlog_graph and use the returned value (the non-None typed graph)
for graph.to_json() (and any other graph usage), and similarly apply the same
guard where regenerate already uses _require_backlog_graph so no code path can
dereference a None graph.
Comment on lines
+114
to
123
| - `specfact project --help` - Initialize project bundle | ||
| - `specfact project --help` - Set active plan (used as default for other commands) | ||
| - `specfact code import [<bundle-name>] --repo <path>` - Import from codebase (uses active plan if bundle not specified) | ||
| - `specfact plan review [<bundle-name>]` - Review plan (uses active plan if bundle not specified) | ||
| - `specfact plan harden [<bundle-name>]` - Create SDD manifest (uses active plan if bundle not specified) | ||
| - `specfact project --help` - Review plan (uses active plan if bundle not specified) | ||
| - `specfact project --help` - Create SDD manifest (uses active plan if bundle not specified) | ||
| - `specfact govern enforce sdd [<bundle-name>]` - Validate SDD (uses active plan if bundle not specified) | ||
| - `specfact sync bridge --adapter <adapter> --repo <path>` - Sync with external tools | ||
| - `specfact project sync bridge --adapter <adapter> --repo <path>` - Sync with external tools | ||
| - See [Command Reference](../../docs/reference/commands.md) for full list | ||
|
|
||
| **Note**: Most commands now support active plan fallback. If `--bundle` is not specified, commands automatically use the active plan set via `plan select`. This improves workflow efficiency in AI IDE environments. |
Contributor
There was a problem hiding this comment.
Stale legacy command reference in active-plan guidance.
This section was updated to specfact project ..., but Line 123 still instructs users to set active plans via plan select, which conflicts with the documented canonical command surface and can send users to removed/legacy syntax.
As per coding guidelines, docs/**/*.md must maintain “User-facing and cross-site accuracy ... CLI examples matching bundled 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 `@packages/specfact-spec/resources/prompts/shared/cli-enforcement.md` around
lines 114 - 123, Update the stale legacy reference to the active-plan command:
replace the bare `plan select` text with the canonical bundled CLI form
`specfact plan select` (or the correct namespaced command used elsewhere, e.g.,
`specfact project plan select` if that is the project-scoped variant) so the
guidance matches the other examples like `specfact project --help`; ensure the
note and any other occurrences in this file use the same fully qualified CLI
command syntax.
Comment on lines
+169
to
+179
| if GENERATED_COMMANDS_PATH.is_file(): | ||
| raw = json.loads(GENERATED_COMMANDS_PATH.read_text(encoding="utf-8")) | ||
| generated_paths: set[CommandPath] = set(CORE_COMMAND_PREFIXES) | ||
| if isinstance(raw, list): | ||
| for entry in raw: | ||
| if not isinstance(entry, dict): | ||
| continue | ||
| command = entry.get("command") | ||
| if isinstance(command, str): | ||
| generated_paths.add(tuple(command.split())) | ||
| return generated_paths |
Contributor
There was a problem hiding this comment.
Fail fast on malformed generated command overview JSON.
If the JSON shape drifts (or decode fails), this path can degrade into misleading command-validation output instead of surfacing the root cause immediately.
As per coding guidelines, scripts/**/*.py: Deterministic tooling: signing, publishing, docs generation; subprocess and path safety.
Suggested fix
def _build_valid_command_paths() -> set[CommandPath]:
if GENERATED_COMMANDS_PATH.is_file():
- raw = json.loads(GENERATED_COMMANDS_PATH.read_text(encoding="utf-8"))
+ try:
+ raw = json.loads(GENERATED_COMMANDS_PATH.read_text(encoding="utf-8"))
+ except json.JSONDecodeError as exc:
+ raise RuntimeError(
+ f"Invalid generated commands JSON at {GENERATED_COMMANDS_PATH}: {exc}"
+ ) from exc
+ if not isinstance(raw, list):
+ raise RuntimeError(
+ f"Unexpected generated commands schema at {GENERATED_COMMANDS_PATH}: expected a JSON list."
+ )
generated_paths: set[CommandPath] = set(CORE_COMMAND_PREFIXES)
- if isinstance(raw, list):
- for entry in raw:
- if not isinstance(entry, dict):
- continue
- command = entry.get("command")
- if isinstance(command, str):
- generated_paths.add(tuple(command.split()))
+ for entry in raw:
+ if not isinstance(entry, dict):
+ raise RuntimeError("Unexpected generated commands entry: expected object records.")
+ command = entry.get("command")
+ if not isinstance(command, str) or not command.strip():
+ raise RuntimeError("Unexpected generated commands entry: missing non-empty 'command'.")
+ generated_paths.add(tuple(command.split()))
return generated_paths🤖 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 169 - 179, The current logic
silently ignores JSON decode errors and malformed shapes when reading
GENERATED_COMMANDS_PATH, which can hide root causes; update the code around
GENERATED_COMMANDS_PATH, raw and generated_paths so it fails fast: wrap
json.loads(...) in a try/except to catch json.JSONDecodeError and re-raise a
clear ValueError (or log+raise) with the path and decode error, assert that raw
is a list and raise if not, and when iterating entries require each entry to be
a dict and that entry.get("command") is a str—if an entry is malformed raise a
ValueError describing the offending entry (rather than continue); keep using
CORE_COMMAND_PREFIXES and GENERATED_COMMANDS_PATH but return generated_paths
only after validation succeeds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Harden module-side command reliability for tester bug reports #586-#592 and the paired core change:
tester-module-cli-reliabilityllms.txtplus generated command JSON/Markdown artifactsSPECFACT_MODULES_ROOTSand packagesrcrootsValidation
openspec validate tester-module-cli-reliability --stricthatch run formathatch run type-checkhatch run linthatch run yaml-linthatch run check-command-overviewhatch run check-command-contracthatch run python scripts/check-docs-commands.pyhatch run python scripts/check-prompt-commands.pyopenspec/changes/tester-module-cli-reliability/TDD_EVIDENCE.mdhatch run contract-test -q-> scoped contract validation passedspecfact code review run --scope changed --bug-hunt --include-tests --json --out .specfact/code-review.changed.jsonrerun after fixesKnown Gate Status
The modules commit was made with
--no-verifybecause Block 2 code review now loads correctly and reports existing legacy clean-code blockers in whole changed files:Review completed with 396 findings (161 blocking)TDD_EVIDENCE.mdThis PR intentionally does not weaken the gate or refactor those unrelated large command implementations.