Skip to content

fix(hooks): key the environment contract by harness so a second CLI never clobbers a live run - #570

Merged
thewrz merged 3 commits into
mainfrom
feat/issue-551
Aug 30, 2026
Merged

fix(hooks): key the environment contract by harness so a second CLI never clobbers a live run#570
thewrz merged 3 commits into
mainfrom
feat/issue-551

Conversation

@thewrz

@thewrz thewrz commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

This was written agentically; verify its assertions:

Why

.agent/env-contract.txt was one untracked file per checkout carrying harness-specific facts (skills= path=, harness= name=). A second harness opening the same checkout rewrote it at SessionStart, silently changing the skills tree, helper paths, and hook verdicts of a run already in flight — observed 2026-08-28 when a Claude session opened to observe a Codex parallel-issues root clobbered its contract mid-wave.

What

  • Every writer now targets its own .agent/env-contract.<harness>.txt (SessionStart passes --write explicitly); the bare name is a read-only legacy fallback for one release and is forwarded into the keyed file on first reuse.
  • contract-read.sh, contract-cache.sh, and the hook guard library resolve the running harness's file first, so the pinned-plugin-path lesson no longer fires against a session's own tree.
  • Observer mode: a session that starts while another harness's keyed contract is still fresh records mode=observer other-harness=<x>; PreToolUse denies-once a content-bearing write into the checkout root from an observer. create-issue-worktree.sh --inherit-session prefers the root's keyed contract.
  • Tests: two SessionStart invocations with different harnesses leave two intact files; observer denial; legacy forward-write.

Decisions

  • Liveness signal is the other harness's contract-file mtime (CONTRACT_MAX_AGE_MINUTES), deliberately not a new heartbeat/lock subsystem; a false positive only costs an unnecessary observer mode.
  • agent-preflight.sh's bare-name --write default and the per-worktree contract flow are unchanged (outside the declared write set; worktrees are single-harness). Consequence: every SKILL.md Step-0 resolver still reads the bare name, which the skill's own preflight still writes — works today; migrating that prose to the keyed name is a follow-up.
    base=origin/main
    files=10
    total.insertions=414
    total.deletions=36
    total.lines=450
    operational.files=10
    operational.insertions=414
    operational.deletions=36
    operational.lines=450
    generated.files=0
    generated.insertions=0
    generated.deletions=0
    generated.lines=0
    lockfile.files=0
    lockfile.insertions=0
    lockfile.deletions=0
    lockfile.lines=0
    fixture.files=0
    fixture.insertions=0
    fixture.deletions=0
    fixture.lines=0
    non_operational.files=0
    non_operational.insertions=0
    non_operational.deletions=0
    non_operational.lines=0

Testing

  • tests/run-tests.sh --only hooks,session-contract-freshness green (new keyed-contract, observer, and legacy-forward assertions)
  • Full agent-run.sh --cmd test ALL GREEN on the final tree (.agent/logs/20260830T052658Z-test.log)
  • CI green on this PR
  • Sol xhigh adversarial review receipt posted

🤖 Co-authored by Claude Fable 5 (root) / claude-sonnet-5 high (worker).

Closes #551

…ever clobbers a live run

The environment contract was one untracked file per checkout, but it carries harness-specific
facts (skills= path=, harness= name=). A second harness opening the same checkout rewrote it
at SessionStart, silently changing the skills tree, helper paths, and hook verdicts of a run
already in flight -- including a false "wrong plugin path" advisory against a path that was
actually correct for the running harness.

Each harness now writes and prefers its own .agent/env-contract.<harness>.txt; the bare,
un-suffixed name is kept as a read-only legacy fallback for one release and is never written
again. SessionStart also records mode=observer (with the harness it is observing) when another
harness's own contract was still fresh at start, and PreToolUse now refuses -- once, retry
allowed -- a write into the checkout root from an observer session, so two harnesses on one
machine no longer fight over the same files.

Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2fe15254-4518-43f8-882f-5aa80ca96efd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

…g them

Adversarial review on PR #570 found two gaps in contract_other_harness_active
(the mode=observer liveness check added by the harness-keyed contract fix):

- F1 [P1]: it accepted ANY readable file matching .agent/env-contract.*.txt with no
  untracked/ownership check, and interpolated the unvalidated filename suffix into
  a grep pattern and then verbatim into this session's own mode=observer line --
  a tracked or otherwise hostile file could inject arbitrary bytes into an
  established contract. Fixed by requiring guard_contract_is_ours before reading
  any candidate, and constraining the harness value (from the filename for a keyed
  candidate, from its harness= line for the legacy one) to the same safe
  single-token vocabulary contract_cache_harness_name itself requires.

- F2 [P2]: the scan only looked at keyed files, so an older run of the other
  harness sitting on only a fresh, untracked bare contract was invisible --
  a new session would record mode=owner and could race it. Fixed by adding the
  validated legacy bare contract as an active-run candidate.

Both add regression tests (a tracked hostile keyed file, a malformed-suffix keyed
file, and a fresh legacy-only contract from the other harness); confirmed all four
new assertions fail against the pre-fix function and pass with it.

Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
@thewrz

thewrz commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator Author

This was written agentically; verify its assertions:

Adversarial review receipt

  • Reviewer: provider=openai; model=gpt-5.6-sol; effort=xhigh; mode=cross-provider (reason: peer CLI present; roster cross-harness candidate)
  • Counts: P1=1; P2=1; total=2
  • Confirmed finding: Other-harness contract scan trusted any file and interpolated an unvalidated suffix — verdict=fixed; fix commit SHA(s)=93ba80a167ccbf70f63c5c95f8461a920faf96c1
  • Confirmed finding: Legacy bare contract not considered as an active other-harness run — verdict=fixed; fix commit SHA(s)=93ba80a167ccbf70f63c5c95f8461a920faf96c1

🤖 Co-authored by Claude Fable 5 (root).

@thewrz

thewrz commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator Author

This was written agentically; verify its assertions:

Review ledger

Machine-readable record of every review already performed on this PR.

{
  "version": 1,
  "pr": 570,
  "repo": "wrzonance/agent-kit",
  "reviews": [
    {
      "kind": "adversarial",
      "provider": "openai",
      "model": "gpt-5.6-sol",
      "effort": "xhigh",
      "mode": "cross-provider",
      "harness": "claude",
      "head_sha": "93ba80a167ccbf70f63c5c95f8461a920faf96c1",
      "covered_heads": [
        "864a9ebe68ad26c5f93a52f5e7ad8e75110b9dcc",
        "93ba80a167ccbf70f63c5c95f8461a920faf96c1"
      ],
      "diff_payload": "wrzonance/agent-kit:570:7081b6b56e25118e2c9b9f89c12bbdc590f0912aacc7469a50e51c4e34835c9f",
      "counts": {
        "p1": 1,
        "p2": 1
      },
      "reviewed_at": "2026-08-30T06:18:49Z",
      "coverage": [
        {
          "sha": "864a9ebe68ad26c5f93a52f5e7ad8e75110b9dcc",
          "reason": "merge-down:a8543f3924bb11a19614b0f511ca44de73b15237",
          "covered_at": "2026-08-30T06:37:50Z"
        }
      ]
    }
  ]
}

🤖 Co-authored by Claude Fable 5 (root).

@thewrz
thewrz marked this pull request as ready for review August 30, 2026 06:38
@thewrz
thewrz merged commit 7198273 into main Aug 30, 2026
4 checks passed
@thewrz
thewrz deleted the feat/issue-551 branch August 30, 2026 06:38
thewrz pushed a commit that referenced this pull request Aug 30, 2026
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.

fix(hooks): env-contract.txt is per-checkout, so a second harness clobbers the live run's contract at SessionStart

1 participant