Adapt Claude feature-critique callbacks - #165
Closed
norvalbv wants to merge 1 commit into
Closed
Conversation
## What changed - add a dormant pure adapter for Claude SubagentStop feature-critique callbacks - derive a privacy-preserving turn work ID and collision-safe callback identity from documented provider identifiers - preserve the direct final-message field as UTF-8, while keeping model, prompt hash, completion time, and transcripts explicitly unavailable - return deterministic capability skips and a mandatory work_unbindable quarantine signal when another hook continued the subagent - reject hostile payload shapes and bound identifiers, callback tuples, and final output before capture ## Scope Exactly two new files under gate-engine/critique/provider-adapters: the Claude adapter and its focused tests. This is dormant adaptation only. It does not install or activate hooks, change provider defaults/configuration, read transcripts, write repository/provider artifacts, create bindings, touch repo-graph, alter reviewer inputs, modify feature-critique guidance/prompts/corpora/results, or run a model benchmark. Runtime activation must wait for a later wrapper that durably consumes work_unbindable quarantine signals. ## Validation - 20 focused Claude adapter tests passed - 74 adapter, normalizer, capture, store, and transcript tests passed - TypeScript, Biome, ESLint, structure, and fan-out checks passed - independent correctness, security/privacy, and strict-scope reviews passed after fixes - verified against the official Claude hooks contract: https://code.claude.com/docs/en/hooks - no model benchmark run; no prompt, corpus, or result hash changed
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
Superseded by #196; its exact content is consolidated into the final QA branch. |
norvalbv
added a commit
that referenced
this pull request
Jul 23, 2026
## Consolidation Final QA branch consolidating Devkit draft PRs #131, #132, #136, #140, #137, #141, #142, #175, #176, #177, #188, #194, #143, #144, #145, #147, #150, #152, #153, #155, #156, #159, #162, #165, #168, #169, #170, #171, #172, #173, and #174 onto current `origin/main`. This is the single review surface for the plan-stage critique evidence loop: shared critique contract and guidance, immutable private evidence, provider capture for Claude/Codex/Cursor, ancestry-safe bindings, capability metadata, retention/cleanup, and shadow commit projections. ## Validation - typecheck, benchmark typecheck, build, and installed-package import smoke passed - deterministic gates passed: structure, lint, clone detection, and staged benchmark tracker - all five Devkit reviewers passed and checkpointed - feature-scoped suite: 31 files / 450 tests passed - lifecycle/install suite: 5 files / 81 tests passed - final full pre-push suite: 153 files passed, 1 skipped; 2,398 tests passed, 5 skipped - no K=3 or model benchmark run was performed The ship commit tree was verified byte-for-byte against the consolidated integration source before publication. The listed drafts will be closed only after the repo-graph final branch is also verified. Unrelated PR #93 is intentionally excluded and untouched. Note: `devkit ship` completed its commit gates and full pre-push suite, but its network handoff exited before publishing. The exact gated commit was then pushed with hooks suppressed to avoid rerunning the already-passed suite. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added Codex support alongside Claude and Cursor for agent skills, agents, and hooks. * Fresh installations target all supported agent surfaces by default, with options to deselect providers. * Existing installations preserve their recorded provider selections during sync, upgrade, repair, and cleanup. * Added provider-native asset synchronization with safer conflict handling and tracked-file protection. * **Bug Fixes** * Improved cleanup and drift detection to preserve user-owned or tracked files. * Enhanced hook registration handling across supported providers. * **Documentation** * Updated setup guidance, command options, Codex hook trust instructions, and critique workflow documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## CI portability follow-up - resolves validated repository and branch mismatches before evidence-root lookup, making clean-HOME behavior deterministic - pins evidence-root identity on Linux and compares stable identity on macOS so delete/recreate races fail closed without blocking replacement - regenerated the two committed runtime copies - validation: formatting and typecheck pass; the complete changed-module surface passes (38/38 tests) - K=3 critique benchmark and final-head CI are still required before merge
norvalbv
added a commit
that referenced
this pull request
Jul 25, 2026
…clobbering each other (sc-1214) (#215) * feat(coverage): devkit-owned coverage runner so parallel agents stop clobbering each other (sc-1214) devkit owns the coverage **gate**; until now it did not own the thing that *produces* what the gate reads. That gap is a bug, because devkit's stated premise — "parallel agents share one working tree" (`cli/lib/ship/ship-branch.sh`) — is exactly the condition under which producing that artifact breaks. Traced from frink ([sc-1214](https://app.shortcut.com/benordlabs/story/1214)), but the defect belongs to every consumer that selects the `coverage` guard. ## The bug vitest derives its coverage temp dir as `resolve(reportsDirectory, '.tmp')` and removes it **twice** per run — `clean()` takes the whole `reportsDirectory` at startup (`clean: true` is the default), and `cleanAfterRun()` takes `.tmp` again at the end. `reportsDirectory` is the only knob that moves any of it; `--shard` merely suffixes `.tmp`. With the conventional `reportsDirectory: './coverage'`, two runs in one checkout share one `.tmp`. Reproduced: the run that dies is the one that started **second**, and what kills it is the first run **finishing** — `cleanAfterRun()` deletes the `.tmp` the second run is still writing into. It fails with `Something removed the coverage directory ... Vitest created earlier` plus an ENOENT unhandled rejection. Net effect: a consumer that selected the fail-CLOSED coverage gate cannot satisfy it *at all* while a sibling agent is testing — and the failure reads like a real regression. ## The fix `devkit coverage-run` (`gate-engine/coverage/produce.mts`). Each run gets `coverage/.runs/<pid>-<ts>-<rand>` as its own reports directory and republishes to `coverage/coverage-final.json`. The key detail: the isolation is applied via **`--coverage.reportsDirectory` on vitest's command line**, which overrides whatever the consumer's `vitest.config` says. So adoption is a one-line script change with **no config edit in any consuming repo**: ```json "test:run:coverage": "devkit coverage-run" ``` Run dirs live under `coverage/` rather than `os.tmpdir()` so publishing is a same-filesystem `rename` (atomic — a reader never sees a torn report) and so the consumer's existing `coverage` gitignore entry already covers them. ### Staying fail-closed *without* letting runs clobber each other Two properties have to hold at once when a run produces no report (failing tests; `reportOnFailure` is false by default): - **Fail-closed.** While `reportsDirectory` was `./coverage`, vitest's startup `rm -rf` had already wiped the old report — so a failed run left no artifact and the gate blocked. Publishing per-run *without* clearing the stale report would silently convert a fail-CLOSED gate into a fail-OPEN one, the precise defect `docs/decisions/coverage-gate.md` exists to prevent. - **No cross-run destruction.** An artifact newer than this run's start belongs to a sibling that succeeded while we were running. Deleting it would let a failing run destroy a passing run's result — reintroducing, at the artifact level, exactly the interference this change removes. So the clear is guarded on the run's start timestamp — an artifact at-or-older than our start predates this run and is stale by definition; a newer one is a sibling's and is put back — and it **claims before it inspects**: the artifact is `rename`d into the run's own directory first, then examined. `stat`-then-`unlink` would be a TOCTOU where a sibling publishes in the gap and we delete the good report it just wrote; `rename` is atomic, so the claim has exactly one winner and nothing arriving afterwards can be destroyed by us. Both refinements came from devkit's own correctness reviewer rejecting earlier cuts of this diff (first an unconditional delete, then the TOCTOU). ### Scope - The runner is **vitest-only** and says so plainly when vitest is absent. The gate stays runner-agnostic — it reads any istanbul-shaped report, so jest/c8/nyc consumers are unaffected and keep using `guard-coverage` as before. - `COVERAGE_FILE` now has one definition shared by producer and gate, so the path written and the path read cannot drift. - The installer is deliberately **not** touched: `patchPackageJson` never overwrites an existing key, and writing a vitest-specific script into arbitrary consumers would be presumptuous. Adoption stays explicit. - `@vitest/coverage-v8` added as a devDep (and vitest 4.1.8 → 4.1.10 to match) so devkit can actually exercise this end to end. Dev-only; `files: ["dist"]` is unchanged. ## Tests `gate-engine/coverage/__tests__/produce.test.mts` — 12 tests: run-dir isolation and uniqueness, `publishCoverage` fail-closed semantics, stale-run pruning, vitest detection, and an end-to-end test that runs two real staggered `devkit coverage-run` processes against a scaffolded consumer project whose config deliberately pins the shared `./coverage`. Two things that make the e2e test real rather than decorative: 1. **The stagger is required.** Two *simultaneous* identical runs finish together and pass even on the broken setup — a test written that way would pass whether or not the bug exists. The fixture uses a fast file (coverage lands in `.tmp` early) plus a slow one to hold run A open, with run B launched partway in. 2. **Verified to fail without the fix.** Removing the `--coverage.reportsDirectory` flag makes the test fail with the exact reported error. Full devkit suite green: 2106 passed, 0 failed. ## Follow-up frink PR #165 currently carries a standalone version of this fix. Once this releases, that PR collapses to the one-line script change and drops its `vitest.config.ts` edit entirely. * fix(coverage): reject the reserved reports-dir flag and fail a run that published nothing (sc-1214) Addresses both CodeRabbit findings on #215 — see the inline replies for where each premise was wrong. - **Reserved flag.** `--coverage.reportsDirectory` in forwarded args is now rejected up front with an explanation. (Not because a caller could redirect the output — vitest's parser rejects the duplicate itself — but because it died on a raw stack trace leaking devkit's internal run directory.) - **A run that published nothing now exits 1.** Covers every route to "no report", including a consumer missing `json` from `coverage.reporter`, which flag-blocking cannot catch. The gate already failed closed on the absent artifact; this moves the diagnosis to where the cause is. - `try/finally` around publish so a throw cannot strand the run directory. Full suite green: 2112 passed.
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.
What changed
Scope
Exactly two new files under gate-engine/critique/provider-adapters: the Claude adapter and its focused tests. This is dormant adaptation only. It does not install or activate hooks, change provider defaults/configuration, read transcripts, write repository/provider artifacts, create bindings, touch repo-graph, alter reviewer inputs, modify feature-critique guidance/prompts/corpora/results, or run a model benchmark. Runtime activation must wait for a later wrapper that durably consumes work_unbindable quarantine signals.
Validation