fix(skills): add Ticket Grouper Rationale self-check (req-tst-001) - #4660
Open
Trecek wants to merge 30 commits into
Open
fix(skills): add Ticket Grouper Rationale self-check (req-tst-001)#4660Trecek wants to merge 30 commits into
Trecek wants to merge 30 commits into
Conversation
…led validate-* skills Ticket Grouper subagents were reasoning about effort-based splitting correctly but emitting a manifest that did not materialize those splits (Group 12 with 3 named pairs collapsed into 1 ticket; Group 13 with 6 medium-effort files named as a single batch). This commit inserts a runtime self-check into Step 7 of each of validate-test-audit, validate-audit, validate-review-decisions: after the grouper returns its manifest, scan each group's Rationale for named or numbered sub-units describing more file-level splits than that group has separate Ticket Group entries for; re-split before proceeding to Step 8. Pattern-matches on Rationale prose, so it stays compliant with REQ-GRP-006 (no rigid per-file cap). Applied identically (byte-identical block) to all 3 skills — they share Step 7 prose today; the new sync test added in the next commit will guard that property. Co-Authored-By: Claude <noreply@anthropic.com>
…ests/skills/conftest.py The three contract test files (test_validate_test_audit_contracts.py, test_validate_audit_contracts.py, test_validate_review_decisions_contracts.py) each carried their own @functools.cache-decorated `_skill_text()` wrapper around `DefaultSkillResolver().resolve(name).path.read_text(...)`, with test_validate_audit_contracts.py's variant missing the explicit encoding="utf-8" kwarg. Move the resolver into a shared, cached `resolve_skill_text(name)` helper in tests/skills/conftest.py (re-exported via __all__ alongside the existing `assert_ticket_grouper_has_*` helpers and `extract_step_section`). Refactor the three per-file `_skill_text()` wrappers to delegate to it. The two existing `assert_ticket_grouper_has_*` helpers now call the shared `_extract_ticket_grouper_section()` helper instead of inlining the "Ticket Grouper" -> "### Step 7" slice. Behavior-preserving: contract test call sites, fixture boundaries, and the effective read_text(encoding="utf-8") semantics are unchanged; the audit file picks up the explicit utf-8 kwarg for free. Co-Authored-By: Claude <noreply@anthropic.com>
…ression Two new tests under tests/skills/ guard the Step 7 'From Ticket Grouper:' block added in the previous commit: test_ticket_grouper_rules_sync.py (medium, smoke-excluded): parametrized over validate-audit and validate-review-decisions, asserts each skill's Step 7 Ticket Grouper block is byte-equal to the validate-test-audit canonical. Catches any future drift that would silently desynchronize the runtime self-check across skills. test_ticket_grouper_live_behavior.py (large, smoke-marked): replay-style regression for the real #4610-producing manifest. Feeds the actual Group 12 (6 high-effort files / 3 named pairs) + Group 13 (6 medium-effort files) manifest text verbatim through the new self-check instructions via `claude --print --dangerously-skip-permissions --output-format json`, then asserts that neither effort tier ends up collapsed into a single ### Ticket Group block in the model's output. Skip-gated on AUTOSKILLIT_TICKET_GROUPER_LIVE_GATE=1 (opt-in), the `claude` CLI being on PATH, and ANTHROPIC_API_KEY/CLAUDE_CODE_OAUTH_TOKEN/ ~/.claude/.credentials.json auth. Wired into the weekly conformance-probes.yml claude-probe job in the next commit. Per-test timeout override of 150s (subprocess 120s) — the prompt carries the full Step 7 block plus both manifest groups and requires the model to reason about a two-tier re-split. Precedent: tests/execution/backends/test_cli_conformance_probes.py (120-240s), tests/server/test_output_budget_e2e.py (4500s). pyproject.toml: opt this single file out of E501 — its _BROKEN_MANIFEST fixture holds the real Group 12 + Group 13 manifest text verbatim from the original #4610-producing run, so its line lengths are intentional fidelity rather than a refactor target. Co-Authored-By: Claude <noreply@anthropic.com>
…eekly claude-probe The new tests/skills/test_ticket_grouper_live_behavior.py exercises the Step 7 self-check against the real #4610-producing manifest via `claude --print` and is the durable regression guarantee that the runtime fix keeps working. It must run unattended, with isolated auth, on a real credential — the existing weekly claude-probe job already has that plumbing (ANTHROPIC_API_KEY / CLAUDE_CODE_OAUTH_TOKEN), so wire it in rather than spinning up a parallel job. Two edits, both scoped to the "Run Claude Code conformance probes" step: 1. Add AUTOSKILLIT_TICKET_GROUPER_LIVE_GATE: "1" to the step-level env block (alongside ANTHROPIC_API_KEY / CLAUDE_CODE_OAUTH_TOKEN) so the opt-in flag travels with the credentials it gates. 2. Add tests/skills/test_ticket_grouper_live_behavior.py to the existing pytest path list — it carries pytest.mark.smoke so it is already selected by the existing -m smoke flag. codex-probe is intentionally not modified: that job exercises the `codex` CLI, not `claude`, and the new test uses `claude --print`. Co-Authored-By: Claude <noreply@anthropic.com>
…oup_helpers The underscore-prefixed module + function signaled package-private, but six unrelated test packages (execution, execution/backends, integration, cli, server, skills) were already importing it. After test_ticket_grouper_live_behavior.py added a seventh cross-domain caller, the leading underscore was actively misleading. Move the file to tests/process_group_helpers.py (public module name; function names keep their prefix because they are private helpers within the shared module, not an importable public API) and update all callers. No behavior change. Resolves cross-domain test coupling noted by PR #4660 review.
…t.py conftest.py is pytest's plugin/hook surface, not an import target. The resolve_skill_text cache, the Step 7 extractor, the effort/floor assertions, and the canonical skill name have been promoted to a real helpers module (tests/skills/_skill_text_helpers.py). Callers now import from there, and conftest.py is back to just fixtures (plus the pre-existing extract_step_section helper used by the investigate-skill tests). Helpers gain names that encode their boundary: - extract_step7_grouper_block # Step 7 'From Ticket Grouper:' block - _extract_pre_step7_grouper_section # Subagent B planning section - assert_ticket_grouper_has_minimum_group_floor - assert_ticket_grouper_has_effort_based_splitting - resolve_skill_text # cache contract documented - CANONICAL_TICKET_GROUPER_SKILL # 'validate-test-audit' Resolves the 'conftest.py becoming a helper library' finding from PR #4660 review, the near-duplicate extractor names, and the hardcoded 'validate-test-audit' canonical in test_ticket_grouper_live_behavior.py.
…date-* SKILL.md The byte-equality sync test (tests/skills/test_ticket_grouper_rules_sync.py) already detects drift between the three validate-* skills, but a casual editor of one SKILL.md had to read the test to learn that the Step 7 'From Ticket Grouper:' block is sync-enforced across all three. Add an HTML comment marker in each skill pointing at the sync test, so an editor sees the constraint from the file they're editing. The marker is byte-equal across all three skills so the existing sync test still passes unchanged.
…safety PR #4660 review noted six live-behavior probe concerns; address each: * Per-feature env var AUTOSKILLIT_TICKET_GROUPER_LIVE_GATE replaced with the sibling-live-probe var CLAUDE_CODE_SMOKE_TEST (already set by the weekly conformance-probes.yml claude-probe job). Drop the bespoke env entry from the workflow. * Symlink ~/.claude/.credentials.json replaced by copying the bytes into tmp HOME with chmod 0o600, so an interrupted cleanup cannot leave a live pointer back into the user's real credential store. * env = os.environ.copy() in _run_claude replaced by an explicit allowlist (HOME, CLAUDE_CONFIG_DIR, PATH, ANTHROPIC_API_KEY, CLAUDE_CODE_OAUTH_TOKEN, LANG and LC_*) so incidental CI/developer credentials cannot leak into the --dangerously-skip-permissions subprocess. * _has_authentication evaluated at module-import time deferred to a function so any env mutation between import and test execution is observed. * Vacuous assertion max(..., default=0) less than 6 passes vacuously on empty output; explicitly assert blocks is non-empty first, and add a second balanced-split bound (less than or equal to len(tier_files) // 2) so a 5+1 or 4+2 regression is caught even when the single-block case is avoided. * file-wide E501 exemption in pyproject.toml extracted the long verbatim manifest to tests/skills/_fixtures/broken_ticket_grouper_manifest.md so the test logic itself stays within the 99-char limit, then dropped the ignore. Register the fixture in test-filter-manifest.yaml so infra/test_manifest_completeness.py stays green. * Documented the --dangerously-skip-permissions + real-credentials combination in the module docstring so a future reader immediately sees that the combination is intentional. Also tighten the Step 7 self-consistency-check bullet by adding a comment explaining why a balanced split is required (rationale text names 'Pair A/B/C' / 'small groups' — a 5+1 split satisfies the headline check but not the rationale intent, per a follow-up investigation of Group 13's ambiguous 'Single batch ticket.' wording).
Adversarial validation of the resolve-review fixes surfaced three
follow-ups:
* (Finding 10, defense/info, '_has_authentication at import time')
Pytest's skipif stores the boolean result at decoration time, so the
function wrapper in the previous fix didn't actually re-evaluate per
test. Replace the misleading wrapper with an inline expression and
update the comment to honestly state that import-time evaluation is
the correct semantics here — CI sets the env vars and resolves
~/.claude/.credentials.json at the job level before pytest is
invoked, so late env mutations are not a realistic concern.
* (Finding 14, bugs/info, 'Group 13 rationale ambiguity — flakiness risk')
Tighten the balanced-split bound to be per-tier instead of
one-size-fits-all: HIGH_EFFORT bounded by len//3 (pairs → max 2 per
block for 6 files; catches the 3+3 regression) and MEDIUM_EFFORT
bounded by len//2 (small groups → max 3 per block; accepts 3+3).
The 6+0, 5+1, 4+2, and 4+1+1 regressions are still caught; 2+2+2 is
the only Group 12-equivalent that passes; 3+3 is correctly accepted
for Group 13.
* (Validation nits from subagent 1)
- tests/process_group_helpers.py: docstring now reflects cross-domain
reach (6 test packages) instead of the stale 'for execution tests'
wording, and points at the original location in git history.
- tests/skills/test_ticket_grouper_rules_sync.py: drop the parenthetical
in the docstring that restates the CANONICAL_TICKET_GROUPER_SKILL
value (cosmetic; the constant is the single source of truth).
targeted suites:
PYTEST_TEST_PATHS=tests/skills,tests/infra,tests/contracts task test-check
→ 8423 passed, 236 skipped, 16 xfailed, 0 failed (TEST_RESULT=PASS)
…re-Step 7 extractor
Resolve F2: replace the loose `find("Ticket Grouper")` anchor with the
specific `**Subagent B — Ticket Grouper**` subsection heading so the slice
boundary cannot shift if "Ticket Grouper" appears earlier in the SKILL.md.
Co-Authored-By: Claude <noreply@anthropic.com>
Resolve F4: change the end anchor from `### Step 8` to `### Step 8 ` so the slice cannot match a future `### Step 80` / `### Step 81` heading. Co-Authored-By: Claude <noreply@anthropic.com>
…cstring Resolve F22 + F3: drop the 7-line docstring on resolve_skill_text down to the single-line summary, and expose ``clear_resolve_skill_text_cache()`` so tests that mutate bundled skill files can drop the cached copy instead of poisoning subsequent callers. Co-Authored-By: Claude <noreply@anthropic.com>
…entials Resolve F8: remove the ``except OSError: pass`` around ``target.chmod(0o600)``. The copy lives in a tmp ``HOME`` so the cost of a hard failure is negligible, and a swallowed chmod could leave the in-test credentials world-readable. Co-Authored-By: Claude <noreply@anthropic.com>
…n _initialize_repository
Resolve F9: replace ``env={**os.environ, ...}`` with a narrow allowlist
(_GIT_SUBPROCESS_ENV_ALLOWLIST) so incidental CI secrets (including any
``*_TOKEN`` or auth-related vars) cannot leak into the local git subprocess
the way they could into the claude subprocess.
Co-Authored-By: Claude <noreply@anthropic.com>
…utput Resolve F12: wrap json.loads in a try/except for JSONDecodeError, KeyError, and TypeError; on failure call pytest.fail with the response tail so the test surfaces the actual stdout instead of crashing with an unhandled exception. Co-Authored-By: Claude <noreply@anthropic.com>
Resolve F11: move ``_cleanup_owned_process_group`` into a ``finally`` block so the live-gate subprocess's owned PGID is reaped on every exit path (success, non-zero returncode, or pytest.fail()), not only on TimeoutExpired. Co-Authored-By: Claude <noreply@anthropic.com>
…pty split Resolve F5 + F14: change the live-gate ``assert blocks`` to ``assert len(blocks) >= 2`` so a single-block or pure-prose response no longer passes vacuously. The fixture manifest already produces two ticket groups (Group 12 + Group 13) so the lower bound is the real one. Co-Authored-By: Claude <noreply@anthropic.com>
…t fixture Resolve F15: add a module-load assertion that every name in ``_HIGH_EFFORT_FILES`` and ``_MEDIUM_EFFORT_FILES`` appears in ``broken_ticket_grouper_manifest.BROKEN_MANIFEST``. The tuples stay explicit (markdown parsing is fragile) but a manifest drift now fails fast instead of silently degrading the tier-bound assertions. Co-Authored-By: Claude <noreply@anthropic.com>
…ture Resolve F10: replace the module-level ``pytest.mark.skipif`` with an ``autouse=True`` fixture ``_require_live_gate`` that re-evaluates the opt-in / executable / auth predicate at test-collection time, so late ``os.environ`` mutations from a session-level fixture or conftest are honoured instead of being masked by an import-time snapshot. Co-Authored-By: Claude <noreply@anthropic.com>
Follow-up to the F10 commit: wrap the chained ``or`` expression in ``bool(...)`` so Pyright sees a pure bool return and stops warning about ``str | bool`` leaking out of an ``and``/``or`` short-circuit chain. Co-Authored-By: Claude <noreply@anthropic.com>
Resolve F20: the module docstring duplicated info already encoded in the ``_require_live_gate`` reason text, the ``_build_subprocess_env`` env allowlist, and the ``_copy_credentials`` helper docstring. Trim down to the one-sentence scope plus a pointer to the helper that holds each piece of detail. Co-Authored-By: Claude <noreply@anthropic.com>
Resolve F23: drop the 9-line rationale for moving helpers to ``_skill_text_helpers.py`` down to a two-sentence summary that still points at the helpers module and the pre-existing in-place helper. Co-Authored-By: Claude <noreply@anthropic.com>
…ure docstring Resolve F21 + F24: the manifest fixture's docstring claimed a ``test_ticket_grouper_live_behavior.py`` E501 ignore lived in ``pyproject.toml`` — but no such entry exists (L131-L151 of pyproject.toml lists E501 ignores only for unrelated test modules). Drop the false claim, split the docstring to a single concern (verbatim fixture + file storage rationale), and remove the now-redundant rationale paragraph. Co-Authored-By: Claude <noreply@anthropic.com>
…lpers.py Resolve F0: the cross-domain rename dropped the leading underscore that marks helper modules throughout this repo (``tests/_codex_feature_policy.py``, ``tests/skills/_skill_text_helpers.py``, etc.). Restore the underscore so the file's helper role is still encoded in its name, and update the single consumer in ``tests/skills/test_ticket_grouper_live_behavior.py``. Co-Authored-By: Claude <noreply@anthropic.com>
… canonical block Resolve F6: the sync test only compared byte-equality across the three SKILL.md Step 7 blocks. If the canonical block's ``Rationale self-consistency check`` bullet were uniformly dropped, the test would silently start passing an empty (or near-empty) common substring. Add a static check that the canonical block actually contains the bullet the rest of the suite is asserting on. Co-Authored-By: Claude <noreply@anthropic.com>
…stency bullet Resolve F7: add ``test_ticket_grouper_has_rationale_self_consistency_check`` to ``TestValidateAuditTicketGrouper`` so the bullet presence is asserted via the contract test path, not just the cross-skill sync test. If the bullet is uniformly dropped from all three validate-* skills in a future edit, both the sync test's byte-equality check and this static check fire. Co-Authored-By: Claude <noreply@anthropic.com>
…assertions Resolve F19: the three per-skill contract test files each contained a ``TestValidate*TicketGrouper`` class with identical ``test_ticket_grouper_has_minimum_group_floor`` and ``test_ticket_grouper_has_effort_based_splitting`` methods (plus the new Rationale self-consistency check from F7). Add ``test_ticket_grouper_contract_shared.py`` with three parametrised tests over ``validate-audit``, ``validate-test-audit``, and ``validate-review-decisions`` so the assertions have a single source of truth; the per-skill classes are kept for now to preserve the existing test IDs and the per-skill grouping for failure triage. Co-Authored-By: Claude <noreply@anthropic.com>
…ment Resolve F17: the comment on ``CANONICAL_TICKET_GROUPER_SKILL = 'validate-test-audit'`` claimed the pinned skill was the first one written, but git history shows ``validate-audit`` (PR #528, 2026-03-26) actually predates ``validate-test-audit`` (PR #1947, 2026-05-05). Replace the inaccurate rationale with an accurate note: the constant is a single byte-equal referent for the sync test, not a historical claim. Co-Authored-By: Claude <noreply@anthropic.com>
After renaming ``tests/process_group_helpers.py`` -> ``_process_group_helpers.py`` for F0, three sibling test files still imported the old name and were silently breaking collection. Update ``test_cli_conformance_probes.py``, ``test_order_blackbox_launch.py``, and ``test_claude_explorer_live_gate.py`` to use the new underscored path. Co-Authored-By: Claude <noreply@anthropic.com>
…low-up to F0) Adversarial validation surfaced two more files still referencing the old ``tests.process_group_helpers`` module path (not caught by the earlier sibling-imports follow-up): * ``tests/execution/test_process_kill.py:30-34`` — both the ``from tests import process_group_helpers as _process_group_helpers`` alias-import and the ``from tests.process_group_helpers import (...)`` named-import. * ``tests/integration/test_codex_startup_canary.py:22`` — the ``from tests.process_group_helpers import (...)`` named-import. Both files would now raise ``ModuleNotFoundError`` at pytest collection. Update them to ``tests._process_group_helpers`` to restore collection. Co-Authored-By: Claude <noreply@anthropic.com>
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
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
Ticket Grouper subagents in the three bundled
validate-*skills were reasoning about effort-based splitting correctly but emitting a manifest that did not materialize those splits — Group 12 with 3 named pairs collapsed into 1 ticket; Group 13 with 6 medium-effort files named as a single batch (real incident that produced #4610).This PR adds a runtime self-consistency check to Step 7 of all three skills (
validate-test-audit,validate-audit,validate-review-decisions) that scans each returned group's Rationale for named or numbered sub-units describing more file-level splits than the group has separate### Ticket Groupentries for, and re-splits before proceeding to Step 8. Pattern-matches on Rationale prose, so it stays compliant with REQ-GRP-006 (no rigid per-file cap).Two new tests guard the fix:
tests/skills/test_ticket_grouper_rules_sync.py(medium) — asserts the Step 7From Ticket Grouper:block stays byte-identical across the three skills.tests/skills/test_ticket_grouper_live_behavior.py(large + smoke) — replays the real Split MEDIUM-effort server test files (6 files batch, ~4900 lines) #4610 manifest verbatim through the new self-check instructions viaclaude --print, asserting neither effort tier ends up collapsed. Skip-gated and wired into the weeklyconformance-probes.ymlclaude-probe job so it runs unattended against real credentials every Monday + onworkflow_dispatch.Requirements
_BROKEN_MANIFESTverbatim from the real run).conformance-probes.ymlwiring).Files Changed
src/autoskillit/skills_extended/validate-test-audit/SKILL.md— primary fix (Step 7 self-check).src/autoskillit/skills_extended/validate-audit/SKILL.md— primary fix (Step 7 self-check).src/autoskillit/skills_extended/validate-review-decisions/SKILL.md— primary fix (Step 7 self-check).tests/skills/conftest.py— addedresolve_skill_text+extract_step7_ticket_grouper_blockhelpers; refactored existing helpers.tests/skills/test_validate_test_audit_contracts.py— refactor_skill_text()to delegate to shared helper.tests/skills/test_validate_audit_contracts.py— refactor_skill_text()to delegate to shared helper.tests/skills/test_validate_review_decisions_contracts.py— refactor_skill_text()to delegate to shared helper.tests/skills/test_ticket_grouper_rules_sync.py(new) — cross-skill byte-equality test.tests/skills/test_ticket_grouper_live_behavior.py(new) — live-behavior regression replaying the real Split MEDIUM-effort server test files (6 files batch, ~4900 lines) #4610 manifest.pyproject.toml— opt the live-behavior test out of E501 (its_BROKEN_MANIFESTis verbatim from the real run, so its long lines are intentional fidelity)..github/workflows/conformance-probes.yml— wireAUTOSKILLIT_TICKET_GROUPER_LIVE_GATE=1and the new test path into the weekly claude-probe job.Implementation Plan
Plan file:
.autoskillit/temp/make-plan/ticket_grouper_behavioral_test_plan_2026-08-16_215858.md🤖 Generated with Claude Code