Skip to content

fix(skills): add Ticket Grouper Rationale self-check (req-tst-001) - #4660

Open
Trecek wants to merge 30 commits into
developfrom
impl-ticket-grouper-20260817-074603
Open

fix(skills): add Ticket Grouper Rationale self-check (req-tst-001)#4660
Trecek wants to merge 30 commits into
developfrom
impl-ticket-grouper-20260817-074603

Conversation

@Trecek

@Trecek Trecek commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

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 Group entries 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 7 From 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 via claude --print, asserting neither effort tier ends up collapsed. Skip-gated and wired into the weekly conformance-probes.yml claude-probe job so it runs unattended against real credentials every Monday + on workflow_dispatch.

Requirements

  • Fix must run on every real production invocation, not just a periodic check — Step 1 (Step 7 self-check, runs on every session).
  • No rigid per-file cap (REQ-GRP-006) — Step 1 (pattern-match on named sub-units, not a numeric cap).
  • Applied identically across all 3 bundled skills — Step 1 (byte-identical block in 3 files) + Step 3 (sync test).
  • Regression test replays the real incident, not an invented one — Step 4 (_BROKEN_MANIFEST verbatim from the real run).
  • Test must not depend on a human running it — Step 5 (weekly conformance-probes.yml wiring).

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 — added resolve_skill_text + extract_step7_ticket_grouper_block helpers; 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_MANIFEST is verbatim from the real run, so its long lines are intentional fidelity).
  • .github/workflows/conformance-probes.yml — wire AUTOSKILLIT_TICKET_GROUPER_LIVE_GATE=1 and 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

Trecek and others added 30 commits August 17, 2026 07:49
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant