From cad39391082ac1c186b9b8e61a570c05fea92615 Mon Sep 17 00:00:00 2001 From: monkut Date: Fri, 31 Jul 2026 13:31:30 +0900 Subject: [PATCH] :bug: Make the Dependencies readiness check advisory instead of blocking `develop` exited 1 when an issue body lacked a `## Dependencies` heading, even when the issue genuinely had no dependencies. The check is a heading regex that never inspects the section contents, so it gated on whether prepare/plan had run rather than on development readiness. - Add `advisory` to `CheckResult`; mark the dependencies check advisory - `_checks_passed()` ignores advisory checks for both the `validate` exit code and the `develop` pre-flight - Report renders a failing advisory check as `WARN` and tallies only blocking checks - Update the plan prompt, bundled skill, and README to match --- README.md | 4 +- askcc/cli.py | 25 ++++++--- askcc/definitions.py | 3 +- askcc/functions.py | 4 +- askcc/skills/handle-github-issue/SKILL.md | 20 +++---- pyproject.toml | 2 +- tests/test_askcc.py | 64 +++++++++++++++++++++++ 7 files changed, 100 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 9e0ab63..3e6484a 100644 --- a/README.md +++ b/README.md @@ -65,7 +65,7 @@ Run `askcc --help` or `askcc COMMAND --help` for the full flag list. |------------|--------------------------------------------------------------------------| | `prepare` | Analyze a backlog issue for development readiness (acceptance criteria, dependencies, estimates) | | `plan` | Fetch the issue and run Claude in planning mode (architecture/design) | -| `validate` | Check issue readiness for development (acceptance criteria, dependencies, assignee, blocking labels) | +| `validate` | Check issue readiness for development (acceptance criteria, assignee, blocking labels; dependencies reported as advisory) | | `develop` | Fetch the issue and run Claude in development mode (implementation) | | `issue-review` | Review issue quality (clarity, completeness, feasibility) | | `pr-review` | Review a PR's code against its linked issue's Definition of Done | @@ -101,7 +101,7 @@ Supporting commands can be used at any point: **Gating mechanisms:** - `prepare` adds the `action:develop` label; subsequent commands require an `action:` label prefix -- `develop` runs readiness validation (acceptance criteria, dependencies, assignee, no blocking labels) before starting +- `develop` runs readiness validation (acceptance criteria, assignee, no blocking labels) before starting; a missing `## Dependencies` section is reported as `WARN` and does not block - The `needs:decision` label blocks `develop` until resolved - `develop` swaps `action:develop` → `action:review` and moves the project board status on success diff --git a/askcc/cli.py b/askcc/cli.py index 2787465..acf1373 100644 --- a/askcc/cli.py +++ b/askcc/cli.py @@ -103,18 +103,29 @@ def _resolve_model(cli_value: str | None, frontmatter_value: str | None) -> str return frontmatter_value +def _checks_passed(checks: list[CheckResult]) -> bool: + """Report whether every blocking check passed. Advisory checks never block.""" + return all(check.passed for check in checks if not check.advisory) + + +def _check_status(check: CheckResult) -> str: + if check.passed: + return "PASS" + return "WARN" if check.advisory else "FAIL" + + def _print_validation_report(issue_url: str, checks: list[CheckResult]) -> None: """Print a structured pass/fail validation report.""" - passed_count = sum(1 for c in checks if c.passed) - total = len(checks) + blocking = [c for c in checks if not c.advisory] + passed_count = sum(1 for c in blocking if c.passed) + total = len(blocking) print(f"Validation Report: {issue_url}") # noqa: T201 print("-" * 60) # noqa: T201 for check in checks: - status = "PASS" if check.passed else "FAIL" - print(f" [{status}] {check.name}: {check.message}") # noqa: T201 + print(f" [{_check_status(check)}] {check.name}: {check.message}") # noqa: T201 print("-" * 60) # noqa: T201 result = "PASS" if passed_count == total else "FAIL" - print(f"Result: {result} ({passed_count}/{total} checks passed)") # noqa: T201 + print(f"Result: {result} ({passed_count}/{total} blocking checks passed)") # noqa: T201 def main() -> None: # noqa: PLR0912, PLR0915, C901 @@ -255,7 +266,7 @@ def main() -> None: # noqa: PLR0912, PLR0915, C901 if args.command == "validate": checks = validate_issue_readiness(args.github_issue_url) _print_validation_report(args.github_issue_url, checks) - sys.exit(0 if all(c.passed for c in checks) else 1) + sys.exit(0 if _checks_passed(checks) else 1) bootstrap_templates() @@ -270,7 +281,7 @@ def main() -> None: # noqa: PLR0912, PLR0915, C901 if action == AgentAction.DEVELOP and not args.skip_validation: checks = validate_issue_readiness(args.github_issue_url) - if not all(c.passed for c in checks): + if not _checks_passed(checks): _print_validation_report(args.github_issue_url, checks) logger.error("Issue readiness validation failed. Use --skip-validation to bypass.") sys.exit(1) diff --git a/askcc/definitions.py b/askcc/definitions.py index 7bcfe21..80ee3e1 100644 --- a/askcc/definitions.py +++ b/askcc/definitions.py @@ -170,7 +170,8 @@ Re-read the body and assignees (`gh issue view --json body,assignees`) and confirm \ (a) `## Acceptance Criteria` heading with a `- [ ]` checklist item, (b) `## Dependencies` (or \ Prerequisites/Context/Blockers) heading is present, and (c) at least one assignee is set. \ -Re-edit / re-assign and re-verify until all three pass — `develop` rejects the issue otherwise. +Re-edit / re-assign and re-verify until all three pass — `develop` rejects the issue without \ +(a) or (c); (b) is advisory but still expected in a planned issue. ## Summary Comment diff --git a/askcc/functions.py b/askcc/functions.py index 1db0f41..921665f 100644 --- a/askcc/functions.py +++ b/askcc/functions.py @@ -303,6 +303,7 @@ class CheckResult: name: str passed: bool message: str + advisory: bool = False # reported but never blocks `develop` or the `validate` exit code @dataclass(frozen=True) @@ -434,13 +435,14 @@ def validate_issue_readiness(github_issue_url: str) -> list[CheckResult]: ) ) - # 2. Dependencies identified + # 2. Dependencies identified (advisory — an issue with no dependencies is still development-ready) has_deps = _has_dependencies_section(body) checks.append( CheckResult( name="Dependencies identified", passed=has_deps, message="Dependencies section found" if has_deps else "No dependencies/context section found", + advisory=True, ) ) diff --git a/askcc/skills/handle-github-issue/SKILL.md b/askcc/skills/handle-github-issue/SKILL.md index f5dfa2a..19f7470 100644 --- a/askcc/skills/handle-github-issue/SKILL.md +++ b/askcc/skills/handle-github-issue/SKILL.md @@ -37,7 +37,7 @@ Run these steps **before** adopting the agent persona below. - Comments: `gh api --paginate repos///issues//comments` - Combine into a single context block: title, body, then each comment as `Comment by @:\n` separated by `---`. 4. **Decision-label pre-check** (all agent actions): see **Decision handling** below — if the `needs:decision` label is already present, stop without posting. -5. **Develop-only readiness gate** (skip with `--skip-validation` if the user says so): run the **`validate`** checks below; if any fail, stop and print the report unless the user explicitly overrides. +5. **Develop-only readiness gate** (skip with `--skip-validation` if the user says so): run the **`validate`** checks below; if any blocking check fails, stop and print the report unless the user explicitly overrides. Advisory checks never stop development. ## Action: prepare @@ -86,14 +86,14 @@ Read current body: `gh issue view --json body -q .body`. Append missing se ## Action: validate -No agent. Run readiness checks and print a structured PASS/FAIL report. Exit non-zero if any fail. +No agent. Run readiness checks and print a structured report. Exit non-zero if any **blocking** check fails. Fetch the issue (`gh api repos///issues/`) and evaluate: -1. **Acceptance criteria** — body contains an `## Acceptance Criteria` (or equivalent) heading **and** at least one `- [ ]` checklist item. -2. **Dependencies identified** — body contains a `## Dependencies` (or `## Prerequisites` / `## Context` / `## Blockers`) heading. -3. **Assignee confirmed** — `assignees` is non-empty. -4. **No blocking labels** — neither `needs:decision` nor `blocked` is present. +1. **Acceptance criteria** (blocking) — body contains an `## Acceptance Criteria` (or equivalent) heading **and** at least one `- [ ]` checklist item. +2. **Dependencies identified** (advisory) — body contains a `## Dependencies` (or `## Prerequisites` / `## Context` / `## Blockers`) heading. Reported as `WARN` when missing; does not affect the exit code. +3. **Assignee confirmed** (blocking) — `assignees` is non-empty. +4. **No blocking labels** (blocking) — neither `needs:decision` nor `blocked` is present. Print a report like: @@ -101,11 +101,11 @@ Print a report like: Validation Report: ------------------------------------------------------------ [PASS] Acceptance criteria: Clear acceptance criteria found - [FAIL] Dependencies identified: No dependencies/context section found + [WARN] Dependencies identified: No dependencies/context section found [PASS] Assignee confirmed: Assigned to: [PASS] No blocking labels: No blocking labels found ------------------------------------------------------------ -Result: FAIL (3/4 checks passed) +Result: PASS (3/3 blocking checks passed) ``` Used as a gate by `develop` (and on user request). @@ -150,7 +150,7 @@ Then **do both**: Apply: `gh issue edit --body ""`. -**Post-update verification** (mandatory before transitioning): re-fetch `gh issue view --json body,assignees` and confirm (a) `## Acceptance Criteria` heading with at least one `- [ ]` item, (b) `## Dependencies` (or Prerequisites/Context/Blockers) heading present, (c) at least one assignee. Re-edit and re-verify until all three pass — `develop` rejects the issue otherwise. +**Post-update verification** (mandatory before transitioning): re-fetch `gh issue view --json body,assignees` and confirm (a) `## Acceptance Criteria` heading with at least one `- [ ]` item, (b) `## Dependencies` (or Prerequisites/Context/Blockers) heading present, (c) at least one assignee. Re-edit and re-verify until all three pass — `develop` rejects the issue without (a) or (c); (b) is advisory but still expected in a planned issue. **Summary comment** — `gh issue comment --body ""` describing sections added/updated and any risks or open questions. @@ -599,7 +599,7 @@ See the **Action selection** table at the top of this file for the per-action `M → Run **prepare** pre-flight → adopt the prepare persona → update description (Acceptance Criteria, Dependencies as drafts) → post summary comment → add `action:develop` label. - **"Validate https://github.com/monkut/askcc-cli/issues/1"** - → Run the four readiness checks → print the report → exit non-zero on any fail. + → Run the four readiness checks → print the report → exit non-zero when a blocking check fails. - **"Plan https://github.com/monkut/askcc-cli/issues/1"** → label-prefix gate → fetch issue + comments → adopt plan persona → rewrite body with Acceptance Criteria / Dependencies / Implementation Plan / Assignee → post-update verification → summary comment → swap `action:plan` → `action:develop`. diff --git a/pyproject.toml b/pyproject.toml index 80a1108..61a9e6d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "askcc" -version = "0.2.14" +version = "0.2.15" description = "A one-shot cc cli executor" authors = [{ name = "mknt", email = "shane.cousins@gmail.com" }] readme = "README.md" diff --git a/tests/test_askcc.py b/tests/test_askcc.py index 95bf842..dea6f17 100644 --- a/tests/test_askcc.py +++ b/tests/test_askcc.py @@ -954,6 +954,24 @@ def test_all_checks_fail(self): assert not any(c.passed for c in checks) assert len(checks) == 4 + def test_dependencies_check_is_advisory(self): + issue_json = self._make_issue_json( + body="## Acceptance Criteria\n- [ ] Item\n", + assignees=[{"login": "dev1"}], + ) + gh_result = subprocess.CompletedProcess(args=[], returncode=0, stdout=issue_json, stderr="") + + with ( + patch("askcc.functions.shutil.which", return_value="/usr/bin/gh"), + patch("askcc.functions.subprocess.run", return_value=gh_result), + ): + checks = validate_issue_readiness(self.ISSUE_URL) + + deps_check = next(c for c in checks if c.name == "Dependencies identified") + assert deps_check.passed is False + assert deps_check.advisory is True + assert [c.advisory for c in checks if c.name != "Dependencies identified"] == [False, False, False] + def test_blocking_label_detected(self): issue_json = self._make_issue_json( body="## Acceptance Criteria\n- [ ] Item\n\n## Context\nSee docs.\n", @@ -1012,6 +1030,28 @@ def test_validate_exits_zero_on_pass(self, capfd: pytest.CaptureFixture[str]): captured = capfd.readouterr() assert "Result: PASS" in captured.out + def test_validate_exits_zero_when_only_dependencies_missing(self, capfd: pytest.CaptureFixture[str]): + issue_json = json.dumps( + { + "body": "## Acceptance Criteria\n- [ ] Item\n", + "assignees": [{"login": "dev1"}], + "labels": [], + } + ) + gh_result = subprocess.CompletedProcess(args=[], returncode=0, stdout=issue_json, stderr="") + + with ( + patch("askcc.functions.shutil.which", return_value="/usr/bin/gh"), + patch("askcc.functions.subprocess.run", return_value=gh_result), + patch("sys.argv", ["askcc", "validate", "-g", self.ISSUE_URL]), + pytest.raises(SystemExit, match="0"), + ): + main() + + captured = capfd.readouterr() + assert "[WARN] Dependencies identified" in captured.out + assert "Result: PASS (3/3 blocking checks passed)" in captured.out + def test_validate_exits_one_on_fail(self, capfd: pytest.CaptureFixture[str]): issue_json = json.dumps({"body": "", "assignees": [], "labels": []}) gh_result = subprocess.CompletedProcess(args=[], returncode=0, stdout=issue_json, stderr="") @@ -1049,6 +1089,30 @@ def test_develop_fails_on_validation_failure(self, capfd: pytest.CaptureFixture[ captured = capfd.readouterr() assert "Result: FAIL" in captured.out + def test_develop_proceeds_when_only_dependencies_missing(self): + """A failing advisory check does not block development.""" + checks = [ + CheckResult(name="Acceptance criteria", passed=True, message="found"), + CheckResult(name="Dependencies identified", passed=False, message="not found", advisory=True), + CheckResult(name="Assignee confirmed", passed=True, message="dev1"), + CheckResult(name="No blocking labels", passed=True, message="none"), + ] + verification_passed = VerificationResult(passed=True, message="skipped", checks=[]) + with ( + patch("askcc.cli.bootstrap_templates"), + patch("askcc.cli.validate_issue_labels", return_value=[]), + patch("askcc.cli.validate_issue_readiness", return_value=checks), + patch("askcc.cli.fetch_github_issue", return_value="issue body"), + patch("askcc.cli.get_runner", return_value=_mock_runner()) as mock_get_runner, + patch("askcc.cli._run_project_verification", return_value=verification_passed), + patch("askcc.cli.transition_issue_to_review"), + patch("sys.argv", ["askcc", "develop", "-g", self.ISSUE_URL]), + pytest.raises(SystemExit), + ): + main() + + mock_get_runner.assert_called_once() + def test_develop_skip_validation_bypasses_check(self): """Develop --skip-validation skips readiness validation and runs Claude.""" verification_passed = VerificationResult(passed=True, message="skipped", checks=[])