Rectify: CmdSpec Argument Ordering Structural Immunity#3341
Conversation
Trecek
force-pushed
the
autoskillit-order-session-sits-idle-tools-variadic-flag-cons/3285
branch
2 times, most recently
from
19801a9 to
579bae0
Compare
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit PR Review — Verdict: approved_with_comments
| claude_code_project_dir, | ||
| get_logger, | ||
| ) | ||
| from autoskillit.execution.backends.codex import CodexFlags |
There was a problem hiding this comment.
[warning] arch: _headless_helpers.py imports CodexFlags directly from execution/backends/codex.py to construct _CODEX_VALUE_BEARING_FLAGS. This couples the headless/ module to a concrete backend implementation. The CodexFlags value-bearing flag constants should be co-located with CodexFlags and imported as a ready-made constant.
There was a problem hiding this comment.
Valid observation — flagged for design decision. The CodexFlags import couples headless/ to a concrete backend. Options: (a) co-locate value-bearing flag constants with each backend, (b) create a backend-agnostic registry in core/types, (c) accept the coupling as pragmatic for the ordering gate.
| from autoskillit.execution.backends.codex import CodexFlags | ||
| from autoskillit.execution.headless._headless_git import _compute_loc_changed | ||
|
|
||
| _CLAUDE_VALUE_BEARING_FLAGS: frozenset[str] = frozenset( |
There was a problem hiding this comment.
[warning] cohesion: _CLAUDE_VALUE_BEARING_FLAGS, _CODEX_VALUE_BEARING_FLAGS, and _ALL_VALUE_BEARING_FLAGS are defined in _headless_helpers.py but belong logically to the command-building layer. They are orphaned from the flag registries they classify and inaccessible to other consumers. Move to backends/_cmd_builder.py or core/types/_type_enums.py.
There was a problem hiding this comment.
Valid observation — flagged for design decision. The value-bearing flag constants are consumed only by assert_interactive_ordering in this file. Moving them to _cmd_builder.py or core/types/_type_enums.py requires evaluating import-layer constraints (IL-0 vs IL-1).
|
|
||
| VARIADIC_CLAUDE_FLAGS: frozenset[str] = frozenset({ClaudeFlags.ADD_DIR, ClaudeFlags.TOOLS}) | ||
|
|
||
| NON_VARIADIC_CLAUDE_FLAGS: frozenset[str] = frozenset( |
There was a problem hiding this comment.
[warning] cohesion: NON_VARIADIC_CLAUDE_FLAGS (core/types/_type_enums.py) and _CLAUDE_VALUE_BEARING_FLAGS (_headless_helpers.py) both classify ClaudeFlags members by argument-taking behavior but are defined separately with overlapping intent and inconsistent membership. The flag-classification vocabulary is split across two layers with neither referencing the other.
There was a problem hiding this comment.
Valid observation — flagged for design decision. NON_VARIADIC_CLAUDE_FLAGS and _CLAUDE_VALUE_BEARING_FLAGS overlap in intent but serve different validation gates. Unification requires deciding on a single taxonomy for flag properties.
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit review: warning-only findings detected. See inline comments — no blocking changes required.
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
Observations accumulated from 1 local review round.
Adds test coverage that will fail until the structural guards are in place: - test_flag_metadata_coverage: closed categorization of ClaudeFlags members - test_cmd_builder: CmdBuilder ordering invariants and CmdSpec origin - test_assert_interactive_ordering: runtime gate for positional/variadic ordering - test_interactive_ordering_gate: AST guard that launch sites call the gate - Extended test_variadic_ordering: cross-backend parametrized coverage Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Introduces typed CmdBuilder, NON_VARIADIC_CLAUDE_FLAGS closed categorization, CmdOrigin provenance metadata, and assert_interactive_ordering runtime gate. - core/types: CmdOrigin dataclass, origin field on CmdSpec, NON_VARIADIC_CLAUDE_FLAGS - execution/backends/_cmd_builder.py: CmdBuilder enforces positional-before-variadic by raising CmdOrderingError immediately if positional() called after variadic_pair() - ClaudeCodeBackend and CodexBackend build_interactive_cmd refactored to use CmdBuilder - _headless_helpers.py: assert_interactive_ordering scans raw cmd tuple, refuses to trust CmdOrigin alone - _session_launch.py and _cook.py: gate wired in before subprocess invocation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… test assertions - Re-export assert_interactive_ordering via execution.headless and execution __init__ - Update imports in _cook.py and _session_launch.py to use public API - Fix error message casing for consistent test matching - Update test_backend_dataclasses to expect origin field and CmdOrigin export - Update headless helpers size budget for new assert_interactive_ordering function - Sort NON_VARIADIC_CLAUDE_FLAGS re-export alphabetically in core/__init__.pyi Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nd remove redundant comment NamedResume session_id is a positional value, not a flag — mode_flag(sid) incorrectly placed it in CmdOrigin.mode_flags. Also removes the slop comment describing absence of plugin_source handling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prevents CmdSpec with empty binary that would fail at subprocess invocation with no diagnostic context. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n ordering test Uses CodexFlags.ADD_DIR when testing CodexBackend. Adds explicit assertion that the variadic flag is present before checking ordering, converting silent skips into visible failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…f cmd index The NamedResume session_id is now tracked in origin.positional (not mode_flags), so tests verify presence and semantic correctness rather than position-dependent indexing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…T import-collection logic Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Trecek
force-pushed
the
autoskillit-order-session-sits-idle-tools-variadic-flag-cons/3285
branch
from
579bae0 to
81ab78e
Compare
Trecek
deleted the
autoskillit-order-session-sits-idle-tools-variadic-flag-cons/3285
branch
1 participant
Summary
ClaudeCodeBackend.build_interactive_cmd()appendedinitial_prompt(a positional argument) after variadic CLI flags (--tools,--add-dir). The CLI parser greedily consumed the prompt string as a flag value, causing sessions to sit idle with a blank prompt. The fix (PR #3302) reordered arguments and addedVARIADIC_CLAUDE_FLAGSmetadata plus ordering tests.The architectural weakness is that
CmdSpecis a flat, opaquetuple[str, ...]with no semantic encoding of argument roles (positional vs flag-value vs variadic). Ordering correctness is enforced only by test coverage — not by construction. This plan introduces a typedCmdBuilderthat makes argument-ordering bugs impossible by construction, plus a runtimeassert_cmd_orderinggate that catches any bypass of the builder.Requirements
Conflict Resolution Table
Implementation Plan
Plan file:
/home/talon/projects/autoskillit-runs/remediation-20260530-130320-729652/.autoskillit/temp/rectify/rectify_cmd_ordering_immunity_2026-05-30_130320.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