Skip to content

feat(acp): harness descriptors, registry, and capability gates as config data - #8255

Open
billygerhard wants to merge 1 commit into
kirodotdev:mainfrom
billygerhard:acp-split/1-core
Open

feat(acp): harness descriptors, registry, and capability gates as config data#8255
billygerhard wants to merge 1 commit into
kirodotdev:mainfrom
billygerhard:acp-split/1-core

Conversation

@billygerhard

@billygerhard billygerhard commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Stack: part 1 of 3 (supersedes #7984) — part 2 wires the registry into
session serving; part 3 lands the Settings UI, docs, and screenshot evidence.
Each part opens after its predecessor merges.

Adding an AI provider to Kiro Crew today means a code PR: a bespoke adapter,
capability sets edited by hand, and a spawn branch per backend. An operator who
runs an ACP-speaking server that Kiro Crew has never heard of — a vendor
adapter binary, an internal gateway, a house-built agent — cannot use it at
all, even though the conversation protocol on the wire is the same standard
ACP the named backends already speak.

Why it matters

The set of ACP-capable providers is growing faster than any hand-written
adapter roster can. Making a provider a ROW OF CONFIG DATA instead of a code
change turns "support my provider" from a feature request into an operator
action, while keeping the security posture explicit: a descriptor declares
what its harness may do, and everything it does not declare stays off.

What changed (motivation → approach → change)

Goal: any ACP provider becomes a row in a dedicated harnesses.json beside
the config. Approach: a validated descriptor object rather than free-form
config — every behavioural difference between harnesses becomes a declared
field with a pinned vocabulary, so the serving code (part 2) can be generic.

This part lands the core, with no consumer wired yet (behaviour of every
existing session path is unchanged — that is part 2):

  • acp/harness_descriptor.py: the HarnessDescriptor dataclass — executable,
    argv template, capability set, MCP delivery mode, model source — with
    field-level validation. Code-only capabilities (internal_sandbox,
    acp_runtime_pool, session_sharing, kiro_identity_store) are REFUSED
    from config with per-key reasons: an operator row can never grant itself a
    sandbox waiver.
  • acp/harness_registry.py: bundled descriptors for the named backends plus
    operator rows read from ~/.kiro/crew/harnesses.json — a dedicated file,
    deliberately NOT a config.json key; invalid rows are excluded from every
    listing with a recorded reason, never silently trusted.
  • acp/protocol_profile.py: each wire dialect pinned as data (protocol
    version, permission-option style, model/effort channel).
  • security.py: harnesses.json is write-protected from agent tools on BOTH
    write paths — the file-edit register and the shell register (a descriptor
    row names an executable the gateway spawns as itself; nothing downstream
    clamps that). Because the file is effectively an execution grant, its leaf
    is additionally matched anchor-independently (_BARE_TOKEN_PROTECTED_LEAVES),
    so a cd-relative redirect cannot walk around the home-anchored spellings.
    Reads stay allowed on both paths: the file holds no secret and the registry
    reads it on every listing. Same relocation pattern as
    playwright-cli-config.json, the computer-use policy, and the OMC rotation
    record.
  • acp/harness_adapters.py spawn-side trust attestation: the resolved
    executable must be a runnable, non-zero-byte file, and the rendered argv is
    refused unless argv[0] execs exactly the attested path.

The parity-gate extension (new CI rules that steer added lines onto
descriptor-declared capabilities) and the harness-parity spec updates ship
with part 2, where the capability-view retirement they describe actually
happens.

Tests

  • test_harness_descriptor.py — field validation, capability refusal from
    config (per-key reasons), delivery-mode vocabulary.
  • test_harness_registry.py — bundled + operator rows, invalid-row exclusion
    with reasons, the harnesses.json file read (valid, malformed, non-object,
    unparseable), and the write-fence grid: file-edit tier membership, shell
    writes refused across every home-prefix spelling AND the bare cd-relative
    form, file-READ tool ungated, Settings-PATCH allowlist pinned.
  • test_harness_generic_adapter.py — executable resolution (absolute,
    PATH-augmented bare name, relative refused, zero-byte refused,
    platform-correct executability).
  • test_harness_attestation.py — the spawn-side trust seam: attested launch
    path, zero-byte and non-executable refusals, and the check that argv[0]
    execs exactly the attested bytes.
  • test_harness_capability_views.py, test_protocol_profile.py,
    test_security.py — leaf/descriptor agreement, dialect profiles, and the
    bare-token fence grid (which is tuple-driven, so harnesses.json runs
    through all five probe families automatically).

Manual verification

N/A for this part — nothing consumes the registry yet, so there is no
user-visible behaviour change; the full stack was hands-on verified in a dev
pod (all three named providers plus a config-authored stub provider serving
real sessions) and that evidence ships with part 3, where the UI lands.

No linked issue: this implements the provider-extensibility work discussed in
#7984, which this stack supersedes.

@billygerhard
billygerhard requested a review from a team as a code owner September 3, 2026 19:29
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running labels Sep 3, 2026
@billygerhard
billygerhard force-pushed the acp-split/1-core branch 2 times, most recently from 48b055f to 29760a7 Compare September 3, 2026 23:38
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@billygerhard billygerhard changed the title feat(acp): harness descriptors, registry, and selection as config data feat(acp): harness descriptors, registry, and capability gates as config data Sep 4, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of ac78f9b23a666e8eb810fb381eaf1fed8d92404b via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound core design — data descriptors, fail-closed capabilities, config-can't-select-code — but live undescribed loader machinery and a docstring-only cross-PR contract need eyes.

Watch

  • Undocumented live scope. The loader/sections stash machinery (_StoredBackendSpelling, _stash_raw_acp_backend writing _acp_backend_stored into the cached/validated dict, acp_backend_alias) runs on every config load today, yet appears nowhere in the description, and the "never serialized back into config.json" property is asserted only in comments — the stash key is planted before the migration write-back and no test in this PR pins that it can't leak to disk.
  • The compat contract is armed by docstring only. registry.default() deliberately ignores acp_backend; the docstring itself names the failure — a part-2 consumer that forgets to compose resolve_alias(acp_backend_alias) silently moves every acp_backend: "kas" operator onto kiro-cli. Nothing in this PR enforces that composition; add a pin or gate before part 2 lands.
  • Shipped operator doc contradicts the pinned vocabulary. harness-authoring.md's field table and worked-example prose say mcp_delivery: "wire"; the validator accepts only wire_fed/file_fed, so an operator copying the reference gets an invalid row.
  • Fence tier vs. the file's stated severity. harnesses.json — "the execution grant", per the PR — is guarded only by command-string matchers (file-edit register + bash leaves), unlike the keystone files which also get OS-sandbox coverage; an interpreter one-liner that never utters the leaf defeats string matching. Before part 2 makes planting exploitable, decide whether the sandbox layer can additionally deny writes (reads staying open rules out the sensitive tier, not that one).

[DESIGN-REVIEWED] ac78f9b

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed ac78f9b23a666e8eb810fb381eaf1fed8d92404b via the fork AI-review pipeline; updated in place on each push.

Review details

FINDING -- src/kiro_crew/security.py:7068 -- "Reads stay allowed on both paths" contradicts the matcher, which denies cat harnesses.json -> Fix: state that only file-read tools and in-process reads remain allowed.
[GPT-REVIEWED] ac78f9b

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of ac78f9b23a666e8eb810fb381eaf1fed8d92404b via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification done. The reused mechanisms (snapshot_trusted_acp_executable, augmented_path, the security registers, selectable_backends, _EDITABLE_CONFIG) all exist in base; the new modules have zero base references; the old capability spellings have ~39 production occurrences across 8 files; base client.py/runtime.py keep 32 _is_claude/PROTOCOL_VERSION sites; base providers.md has no harness-registry or "Chat composer" section.

First-Principles-Verdict: CONCERNS

The core earns its stacked, consumer-less landing — but agent.default_harness and the raw-backend stash are undeclared one-way-door config surface with zero consumers, deferrable to part 2 for free.

What this change ships

Intent: let an operator add any standard-ACP harness as config instead of a code PR — an ADDITION, part 1 of a declared 3-PR stack (zero consumers is derived from the 1 MB review-lane limit).

  1. Operators author harnesses in harnesses.json, invalid rows excluded with reasons — justified
  2. New Settings key agent.default_harness + baseline/schema rows — undeclared, zero consumers, help text describes behavior this build doesn't perform
  3. Shell commands naming harnesses.json (even cat, anywhere) now refused; file-edit writes fenced — justified (execution-grant boundary, fence-before-capability)
  4. Bundled harness capabilities restated as descriptor data — second spelling of the ACP_BACKENDS_* sets (39 occurrences, 8 files still on the old one), drift-pinned, retirement declared part 2
  5. Wire dialects pinned as ProtocolProfile data — parallel to 32 live _is_claude/PROTOCOL_VERSION sites; two fields' own docstrings say "not consumed in production today"
  6. Uniform spawn attestation (resolve → attest → argv check) — justified, reuses snapshot_trusted_acp_executable
  7. Probe-failure memory with 5-min TTL — undeclared, zero callers ("called by the spawn path", which lands in part 2)
  8. Loader smuggles pre-clamp acp_backend via _StoredBackendSpelling/acp_backend_alias — undeclared, zero consumers
  9. legacy_backend_for/harness_for_backend bridge in acp/types.py — undeclared, test-only consumers
  10. New harness-authoring.md + README row rewrite — row and "§ Chat composer" link describe providers.md content absent from this tree

Watch

  • The BLOCK bar ("one-way-door surface, zero consumers") is literally met by item 2; only the declared, sequential stack — part 2 is the consumer — lowers it to CONCERNS. If part 2 stalls, a release ships a dead Settings key whose help text is false.
  • Items 4 and 5 mean this tree carries two maintained spellings of capabilities and of the wire until part 2 lands; the drift pins are the only thing holding them together.
  • README now claims providers.md covers "the harness registry"; grep of base providers.md finds no such section — the index misdescribes a doc the description itself defers to part 2.

Subtractions

  • Defer agent.default_harness entirely (sections.py field, _normalize_harness_id, both config-baseline.json hunks) to part 2, where registry().default() gains its first caller.
  • Defer the stash machinery — _StoredBackendSpelling, _stash_raw_acp_backend, _stored_acp_backend, acp_backend_alias (0 consumers; grepped acp_backend_alias in src: only its definition) — to part 2's session wiring.
  • Drop HarnessAdapter.post_initialize and the permission_option_style/emits_thought_chunks fields — each self-documents zero consumers ("declared anyway" / "No production code consumes this yet"); land them with their first reader.

[FIRST-PRINCIPLES-REVIEWED] ac78f9b

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed ac78f9b23a666e8eb810fb381eaf1fed8d92404b via the fork AI-review pipeline; updated in place on each push.

Review details

The single candidate depends on a live consumer of the registry's availability/resolve path. Confirmed: every caller of resolve_executable / resolve_spawn_executable / adapter_for / HarnessRegistry.list / require_available in this diff is either inside the new modules or in the new test files (lines 3024+ are all test_*.py). No production spawn or listing path (provider factory, acp/client.py, any dashboard handler, /api/harnesses) is touched — the docs themselves defer session selection and the harness surfaces to a later "part 2/part 3." The stored agent.default_harness is normalized shape-only and never reaches a spawn. So ClaudeAdapter.resolve_executable's plain-PATH inheritance is only observable in tests; there is no concrete input → call path → wrong outcome in shipped behavior. (b) and (c) fail — dropped.

No further grounded defect surfaced in the changed lines.

No findings.

[OPUS-REVIEWED] ac78f9b

@billygerhard

Copy link
Copy Markdown
Contributor Author

Dispositions for the GPT 5.6 review of e866930 (addressed in 6bbdec2):

BLOCKING — cli_config.py protected-key deletion check races the locked write: FIXED. The guard is now evaluated a second time INSIDE each lock-held mutate callback (--file replace, --local overlay, and base-key branches), aborting via a sentinel exception so update_config_locked unwinds without writing. The pre-lock check is kept as the no-lock fast path. The stale docstring note that declared the race an accepted residual is rewritten to describe the two-phase check. Pinned by a new race-simulation test (test_config_set_delete_guard_holds_when_the_definition_lands_mid_write) that blinds the pre-lock read while the real file holds a descriptor — the in-lock check must refuse, and does.

FINDING — validation.py accepts a harness field nothing forwards: FIXED as suggested. The validation.py changes (the harness FieldSpecs and the id-grammar regex) are removed from this PR entirely and ship in part 2 together with the consumer wiring that forwards and persists the field — nothing in part 1 now accepts an input it ignores.

Also in 6bbdec2: no other code changes; the diff delta vs e866930 is exactly the cli_config fix, its test, and the validation.py removal.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@billygerhard

Copy link
Copy Markdown
Contributor Author

Disposition for the Design Review BLOCK on 1b9d9f1 (addressed in 104ba08):

ACCEPTED and fixed with the relocation the review prescribed. Operator harness descriptors no longer live in config.json at all. They move to a dedicated harnesses.json beside the config, following the repo's established pattern for values that are inputs to security decisions (playwright-cli-config.json, the computer-use policy, the OMC rotation record):

  • harnesses.json is now in both write-protection registers: _WRITE_PROTECTED_HOME_PATHS (file-edit gate) and _WRITE_PROTECTED_BASH_LEAVES (shell redirects — the verb-independent matcher, so tee/cp/any novel write verb naming the leaf is refused). Reads via the file-READ tool stay open; the file holds no secret.
  • The registry reads the file directly (HarnessRegistry._operator_section), fail-safe: an absent file is the common case, an unparseable one costs only the operator rows, and every row is still validated as untrusted.
  • The review's observation that the relocation "deletes the new _WRITE_PROTECTED_CONFIG_KEYS guard family this PR adds to compensate" is confirmed: the whole key-level setter guard (protected_config_write, the cli_config sentinel plumbing, and its test class) is REMOVED — cli_config.py and test_security.py are now untouched relative to main. The diff got ~30KB smaller.
  • agent.default_harness stays a config key deliberately: it selects among already-registered ids and an unknown/unavailable value degrades to kiro-cli, so the loader-clamp reasoning that protects the rest of config.json holds for it. The docs (harness-authoring.md, security.md, providers.md) now document the dedicated file and the reasoning.

New/updated tests: the shell-fence grid for harnesses.json (write verbs refused across every home-prefix spelling, unrelated names unfenced, file-READ tool ungated), the file-edit-tier membership pin, registry reads from the file (valid, invalid-row, non-object, unparseable), and the Settings-PATCH allowlist pin. 2,107 tests green on this slice locally.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 4, 2026
@billygerhard

Copy link
Copy Markdown
Contributor Author

Dispositions for the GPT 5.6 review of 104ba08 (addressed in c4d5598):

BLOCKING — check_harness_parity.py:106 suppression matched inside string literals: ACCEPTED, fixed. Confirmed: SUPPRESSION.search ran on the raw line, so a marker-shaped STRING LITERAL (log("# harness-ok: x")) silenced the whole line with no visible comment — exactly the bypass the SUPPRESSION docstring claimed was impossible. Fix as prescribed: suppression now reads only the quote-aware trailing comment, via a new comment_part() that is the exact complement of the existing code_part() quote tracker (one mechanism, both directions). Two new self-test probes pin it: the full marker shape inside a string does NOT suppress (full-marker-in-string-does-not-suppress), and a line carrying both an in-string decoy and a genuine trailing marker still suppresses (real-trailing-marker-still-suppresses-past-string). All 41 gate self-test probes green.

BLOCKING — :241 VOCABULARY_PATHS exemption: no change, per the adjudication's downgrade, whose reasoning matches the design: the two exempted modules are themselves vocabulary homes (types.py defines ACP_BACKENDS_ACP_RUNTIME; the descriptor owns CAPABILITY_*), the runtime ceiling (acp_backend not in ACP_BACKENDS_KNOWN refusal) is independent of this definition-location rule, and narrowing to the leaf would flag the PR's own legitimate definitions absent per-symbol exemption machinery.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 4, 2026
@billygerhard

Copy link
Copy Markdown
Contributor Author

Disposition for the GPT 5.6 review of c4d5598 (addressed in 3b3eab4):

BLOCKING — security.py: cd-relative shell writes bypass the harnesses.json fence: ACCEPTED, fixed as prescribed. Confirmed: every _WRITE_PROTECTED_BASH_LEAVES pattern is home-anchored, so a cd into the crew home followed by a relative redirect onto harnesses.json named no home, no prefix, no separator — and reached the file. harnesses.json is now in _BARE_TOKEN_PROTECTED_LEAVES, matched anchor-independently as a bare path segment. It passes both admission tests that register's SCOPE note sets (now stated explicitly as criteria): (1) severity — the filename IS the execution grant (a shape-valid row names a binary the gateway spawns as itself, no downstream clamp), so anchoring cannot be part of the contract, exactly the alias-record class; (2) distinctiveness — a coined leaf ordinary command lines never utter, so the false-positive cost stays confined. Notably this is also why playwright-cli-config.json stays anchored-only (its fence is already conceded by the PLAYWRIGHT_MCP_CONFIG env override) while harnesses.json has no such override, so the widening buys a real closure.

Tests: the existing bare-token grid is tuple-driven, so all five probe families (cd-relative redirect, bare-name-any-verb, subdir-relative, Windows-relative, unrelated-names-stay-allowed) now run against harnesses.json automatically; the harness fence test additionally pins tuple membership and the exact cd-relative shape from the finding. 2,474 tests green across the security/registry/alias/denied-commands suites.

…fig data

Part 1 of 3 (core): author any ACP provider as a config-authored
HarnessDescriptor; the registry validates and serves descriptors, protocol
profiles pin each wire dialect, spawn-side attestation refuses unvetted
executables, and code-only capabilities are refused from config - with a
key-level config-write guard for agent.harnesses. No consumer is wired yet -
session selection and serving are part 2; the UI is part 3.
@billygerhard

Copy link
Copy Markdown
Contributor Author

Dispositions for the First Principles BLOCK and GPT 5.6 review of c4d5598 (addressed in ac78f9b):

FP BLOCKER 1 — spec + gate assert a retirement this commit does not perform: ACCEPTED, fixed by the prescribed subtraction. scripts/check_harness_parity.py and docs/system-specs/modules/harness-parity.md are now UNTOUCHED relative to main in this PR — the gate extension (positive-constant, capability-view-membership rules) and the H6/H9 spec rewrites move to part 2, the PR where AcpRuntime.spawn descriptor rendering and the capability-view retirement actually land, together with test_harness_spawn.py (already in part 2's slice, which is why the "pinned by" reference dangled here). Part 1's added lines pass main's gate as-is, verified locally against the merge-base.

FP BLOCKER 2 — description describes a mechanism the diff replaced: ACCEPTED, fixed. The PR body is rewritten: the stale agent.harnesses key, cli_config.py key-level guard, and config-setter refusal matrix bullets are gone; the security section now describes what actually ships — the dedicated harnesses.json store, both write registers, the anchor-independent bare-token matching, and the tuple-driven fence grid. A "no linked issue" line is added per the readiness notice.

FP subtraction (black reformat rides along): ACCEPTED, fixed. security.py is restored to main's spelling and only the functional fence edits are re-applied — the diff there is now +40/−10 (was ~320 changed lines) and the black-baseline file is untouched.

FP subtractions on _UNSERVICEABLE / ProtocolProfile.permission_option_style / .emits_thought_chunks: no change, declared scope. These are the data spellings part 2's generic serving consumes (its slice is built and verified); dropping and re-adding them across two sequential PRs of one declared stack buys review noise, not smallness. Both docstrings already carry the consumer-lands-with-part-2 declaration, and the advisory Watch items (agent.default_harness, harness-authoring.md) keep their part-2/3 deferral notes.

GPT BLOCKER — suppression marker matched inside string literals: fixed at the source, ships with the gate in part 2. The quote-aware fix (suppression read only from the true trailing comment via comment_part(), the complement of the existing quote tracker) plus two new self-test probes are committed on the feature branch and ride with the gate extension to part 2 — this PR no longer modifies the gate at all, so main's gate (whose suppression pattern this PR never changed) governs it.

Also in ac78f9b: rebased onto current main. 2,350 tests green on the slice; flake8/isort/mypy/black-gate/docs-lint/parity(main's gate)/boundary all pass.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 4, 2026
@NicholasRBowers NicholasRBowers added the needs-pr-triage PR scanner: awaiting automated triage label Sep 8, 2026
@chenmingwei23 chenmingwei23 removed the needs-pr-triage PR scanner: awaiting automated triage label Sep 8, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Attempted a maintainer rebase onto main acc99f21 as part of the 2026-09-08 open-PR audit, then aborted and left this branch untouched: one conflict needs your decision.

src/kiro_crew/security.py was deleted by #9183, which split it into src/kiro_crew/security/. The harnesses.json entry in _WRITE_PROTECTED_HOME_PATHS relocates cleanly to security/paths.py, but _WRITE_PROTECTED_BASH_LEAVES and _BARE_TOKEN_PROTECTED_LEAVES were removed on purposetest/test_security.py::test_the_path_matchers_are_absent asserts both names are absent, so reinstating them fails main. Your test/test_harness_registry.py asserts membership in both. The shell-side fence needs re-arguing under main's model (write-protected home paths plus the OS sandbox), which is your call, not a mechanical resolution.

Also resolved mechanically once you rebase: config/loader.py (main aliases sections as _sections, not _config_sections) and docs/system-specs/modules/README.md (main added two provider rows).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: passed Eligible automated validation passed for the current revision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants