Rectify: check_review_posted Step Wiring#4205
Merged
Trecek merged 15 commits intoJul 8, 2026
Merged
Conversation
…t A) Closes the recurring defect class where Codex sandbox blocks undeclared operation classes. Adds `github_api_write` capability with `required_sandbox_overrides` wired through the full dispatch chain: SKILL.md → run_skill aggregation → SkillSessionConfig → _apply_config → CodexBackend.build_skill_session_cmd → _codex_exec_base extra_overrides → codex exec --config sandbox_workspace_write.network_access=true Also generalizes capability routing from hardcoded "git_metadata_write" name checks to registry-driven checks (any cap with non-empty required_backends), making the routing machinery self-extending. Adds `github_api_callable: bool = False` to BackendCapabilities, declares `github_api_write` on review-pr/SKILL.md, and adds merge-prs to the admission dispatch agreement test. Tests T-A1 through T-A11 added. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ation - Extract generator expressions from run_skill() handler body to standalone helper functions (_aggregate_sandbox_overrides, _has_routing_capability, _get_routing_caps) to satisfy REQ-CNST-008 no-business-logic-in-handlers rule - Add network_access: bool = False to InMemoryHeadlessExecutor.run() and ExecutorCall to match updated HeadlessExecutor Protocol signature - Update test_backend_capabilities field sets to include github_api_callable - Update SkillSessionConfig exhaustive field test to include network_access - Add github_api_callable to NOT_YET_LIVE and CAPABILITY_CLASSIFICATION in conformance tests - Update test_run_skill_references_git_metadata_write_capability to check for registry-driven routing pattern (_has_routing_capability / required_backends) - Bump line budgets: tools_execution.py (1260 to 1295), codex.py (1150 to 1155), headless/__init__.py (510 to 520) to account for new implementation lines - Declare github_api_write in uses_capabilities for 16 skills that make mutating GitHub API calls (Part A of the 17-skill declaration set) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ate routing predicates
Add worker_routable: bool = False field to SkillCapabilityDef to distinguish
between two distinct not-applicable behaviors:
- worker_routable=True (git_metadata_write): REROUTE to claude-code subprocess
→ required_backends=frozenset() so compat gate does not fire
- worker_routable=False (open_kitchen, github_api_write): REJECT via compat gate
→ required_backends={claude-code} for not-applicable, frozenset() for others
Update _has_routing_capability and _get_routing_caps in tools_execution.py to
check worker_routable instead of required_backends so open_kitchen skills are
no longer silently rerouted to claude-code.
Update both _auto_overrides.py sites (capability-route disjunct in
_provider_aware_capability_overrides and _compute_effective_backend_map) to
use worker_routable, and fix stale docstring references to git_metadata_write.
Update test_codex_status_consistent_with_required_backends for new semantics
and add T-N4 invariant tests (worker_routable→not-applicable+empty-backends,
not-applicable+!worker_routable→claude-code). Update AST guard in
test_capability_admission_control.py to check for worker_routable instead
of required_backends in tools_execution.py.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pabilities Collapse shell line continuations (re.sub r'\\n\s*' to space) in _detect_capabilities so multi-line gh api calls (e.g., gh api \ --method POST) are detected by the github_api_write pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
audit-review-decisions, resolve-research-review, and file-audit-issues all perform mutating GitHub API calls (gh api --method POST, createIssue, addLabelsToLabelable) but did not declare github_api_write. This caused the forward-check test to flag undeclared capability usage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… in review-pr When gh api exits with a network/connection error (e.g., curl: (7) Failed to connect) in Codex sandbox, set verdict=needs_human and exit 0 rather than crashing. The needs_human verdict is in _SAFE_DEGRADATION_VERDICTS so the verdict-ungated-degradation rule does not fire. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ion negative test T-N1a (test_github_api_write_capability_skill_not_auto_routed): verifies that github_api_write (fix-required, worker_routable=False) does not trigger auto-routing, runs successfully on Codex, and the executor receives network_access=True from required_sandbox_overrides. T-N1b (test_git_metadata_write_capability_still_routed): verifies that git_metadata_write (not-applicable, worker_routable=True) still auto-routes to claude-code after the discriminator fix, emitting backend_override_activated log. T-N2 (test_open_kitchen_capability_not_routing_eligible_in_admission): verifies that _compute_effective_backend_map does not route open_kitchen-capability steps to claude-code — open_kitchen is not worker_routable and must stay on the codex backend in the effective backend map. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…thub_api_write - Add github_api_write to resolve-claims-review uses_capabilities - Update backend compat tests: git_metadata_write is now worker_routable=True so required_backends=frozenset(); compat gate does not fire for routable caps - test_codex_backend_fires_without_effective_backend_map renamed to test_codex_backend_no_compat_findings_for_routable_skills; asserts 0 findings - test_compute_capability_feasibility: valid=False changed to valid=True - test_recipe_backend_matrix: valid=False/errors>=1 changed to valid=True/errors==0
Installs the review effect verification layer described in the plan:
- Test hermeticity: add _clear_run_skill_env autouse fixture to conftest.py
so AUTOSKILLIT_RUN_SKILL__* env vars injected by the pipeline harness
don't leak into config tests (fixes 4 failing tests)
- check_review_posted callable: smoke_utils._review.check_review_posted
reads batch_review_response_{pr_number}.json from output_dir (github mode)
or always passes (local mode); exported from smoke_utils.__all__
- Callable contract: skill_contracts.yaml entry with inputs/outputs spec
- BFS API: bfs_reachable_without_barrier now accepts str | frozenset[str]
barrier; docstring corrected (barrier IS visited but not expanded)
- Semantic rule: review-effect-verification-waypoint fires when any advance
gate (check_review_loop or derive_batch_ci_event) is BFS-reachable from a
review-pr skill step without crossing check_review_posted
- Recipe insertions: check_review_posted run_python step added to all 4
recipes (implementation, remediation, implementation-groups, merge-prs);
review_pr and re_push_review success routes updated to check_review_posted
- SKILL.md: receipt file write (batch_review_response_{pr}.json) after
successful gh api batch review POST
- Diagram hashes updated for all 4 modified recipes
- contract cards and compiled JSON regenerated via task regen-contracts &&
task compile-recipes
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add missing type: str to check_review_posted callable contract outputs - Remove erroneous skip_when_false: inputs.open_pr from check_review_posted in implementation, implementation-groups, and remediation recipes - Add on_failure to check_review_posted in all 4 recipes - Fix /autoskillit:review-pr to /review-pr prefix in rules_graph_review.py - Update test_implementation and test_remediation to assert check_review_posted - Update _LEGACY_JSON_WRITES line numbers (317->341, 409->433) - Add check_review_posted to smoke_utils exports completeness test - Update diagram hashes for all 4 modified recipes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…art A)
Fixes RC1–RC4a found on the combined merged tree:
- RC1: Remove optional: true (without skip_when_false) from check_review_posted
in implementation, implementation-groups, and remediation recipes.
- RC2: Add catch-all on_result arm to check_review_posted in all 4 bundled
recipes to satisfy unrouted-callable-verdict rule.
- RC3: Add work_dir: ${{ context.work_dir }} to check_review_posted.with in
merge-prs to satisfy run-python-requires-work-dir rule.
- RC4: Fix _review_effect_verification_waypoint step selector to use
resolve_skill_name() == "review-pr" instead of .startswith("/review-pr"),
making the rule match /autoskillit:review-pr canonical form.
- RC4a: Route re_push_review_integration.on_success to check_review_posted
(not derive_batch_ci_event) in merge-prs, closing the bypass on the
rebase-then-repush path.
Updates compiled .json siblings, diagram hash comments, and contract cards.
Adds test B22b (re_push_review_integration gates through check_review_posted)
and B22c (check_review_posted routes to derive_batch_ci_event on true).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… (issue #4204 Part B) - Add RecipeStep.skill_name @Property that calls resolve_skill_name — makes canonical normalization discoverable via IDE autocompletion on step. - Extract _get_review_pr_steps(ctx) from the inline comprehension in _review_effect_verification_waypoint — now independently testable. - Add tests/recipe/test_recipe_step_skill_name.py (5 tests covering both prefix forms, None returns, and different skills). - Update silence tests in test_rules_review_effect_waypoint.py to assert _get_review_pr_steps returns >= 1 steps before asserting 0 findings — structurally prevents vacuous silence. - Add test_review_effect_waypoint_selects_steps_on_bundled_recipes parametrized over 4 bundled recipes as an early-warning canary. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_net = cfg.get("network_access", False) was a no-op — never consumed by
build_skill_session_cmd or any downstream caller. Claude Code has native
network access via CLAUDE_CODE_CAPABILITIES; no sandbox override needed.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d docstring The docstring said the is_absolute() guard was "required by tests/arch/test_run_python_path_resolution.py" — inverting causality. The guard exists because absolute paths are architecturally correct; the test merely verifies the convention. Embedding a test filename as justification will rot if the test is renamed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ing removal Removing the two-line test-file parenthetical from check_review_posted docstring shifted the json.dumps call sites in _review.py by -1 line. Update _LEGACY_JSON_WRITES entries from 341/433 to 340/432. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Trecek
deleted the
review-pr-on-codex-workers-is-a-silent-no-op-sandbox-blocks/4204-4
branch
This was referenced Jul 8, 2026
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
Parts A and B of issue #4204 introduced the
check_review_postedstep andreview-effect-verification-waypointrule across all 4 bundled recipes, but the combined merged tree has 74 failing tests from 5 mechanical defects introduced by an incomplete round-3 remediation (fe8931e8f):optional: truewithoutskip_when_falseleft oncheck_review_postedin 3 recipes (implementation, implementation-groups, remediation) →optional-without-skip-whenERROR cascades to 70+ testscheck_review_posted.on_resultin all 4 recipes →unrouted-callable-verdictERROR (sentinel outputno_reviews_postedis unrouted)check_review_posted.output_dirin merge-prs uses a static relative path ({{AUTOSKILLIT_TEMP}}/review-pr, no${{template) with nowork_dir→run-python-requires-work-dirERROR_review_effect_verification_waypointstep-selection uses.startswith("/review-pr")— never matches/autoskillit:review-prforms → rule permanently silent → 3 "fires" tests failre_push_review_integration.on_success: derive_batch_ci_eventin merge-prs bypassescheck_review_postedon the rebase-then-repush path → will cause the rule to fire on merge-prs once RC4 is fixedPart A eliminates all 5 defects. Part B (separate task, explicit authorization required) adds architectural immunity to prevent recurrence.
Note on merged tree state: After merging both impl branches,
_analysis_bfs.pyalready supportsbarrier: str | frozenset[str](updated by Part B), so thefrozensetbarrier call in_review_effect_verification_waypointis correct and requires no change. Thecheck_review_postedcallable, itsskill_contracts.yamlentry,smoke_utilsexports, andtests/test_smoke_utils.pyare all present and passing. Only the 5 defects above need repair.Closes #4204
Implementation Plan
Plan file:
/home/talon/projects/autoskillit-runs/remediation-20260707-160731-479581/.autoskillit/temp/rectify/rectify_check-review-posted-wiring_2026-07-07_163000_part_a.md🤖 Generated with Claude Code via AutoSkillit
Token Usage Summary
* Step used a non-Anthropic provider; caching behavior may differ.
Token Efficiency
Model Usage Breakdown