Repository navigation
feat: add native plugin capability tour with lifecycle hook proof - #34
Conversation
…ope to candidate lineage (U4)
GitHub Actions
Inactive
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds a Claude and Codex capability tour with native lifecycle hooks, deterministic fixtures, bounded payload validation, deterministic packaging, installed capability evidence, and candidate-bound qualification lineage. ChangesNative capability tour
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Candidate
participant ship-canary
participant PluginArchive
participant InstalledClients
participant HarnessProof
Candidate->>ship-canary: Qualify exact source commit
ship-canary->>PluginArchive: Build deterministic archive and payload hash
ship-canary->>InstalledClients: Install and compare Claude/Codex payloads
InstalledClients-->>ship-canary: Return installed payload hashes
ship-canary->>HarnessProof: Validate candidate lineage and capability evidence
HarnessProof-->>Candidate: Return bound qualification evidence
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (15)
plugin/skills/capability-tour/SKILL.md (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the skill list from the observed catalog.
Line 12 instructs the model to read the installed skill catalog. Line 42 then hardcodes five skill names. A template consumer who renames or removes skills through
initgets a response that contradicts the catalog it just read. State the expected default set as an example, and require the reported list to come from the catalog.♻️ Proposed wording change
-3. `Available portable skills`: list `hello-world`, `skill-a`, `skill-b`, `runtime-custody`, and `capability-tour`, marking the first three Bun-backed and the last two model-only. +3. `Available portable skills`: list the skills found in the installed catalog, marking each Bun-backed or model-only. The unmodified template ships `hello-world`, `skill-a`, and `skill-b` as Bun-backed plus `runtime-custody` and `capability-tour` as model-only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/skills/capability-tour/SKILL.md` at line 42, Update the “Available portable skills” instruction to require deriving the reported skills from the installed catalog read earlier, rather than hardcoding names. Mention hello-world, skill-a, skill-b, runtime-custody, and capability-tour only as the expected default set, while preserving the Bun-backed versus model-only classifications.scripts/native-capability-fixture.ts (1)
71-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMatch the write behavior of
writeGeneratedFiles.
writeGeneratedFilesinscripts/plugin-config.tslines 474-483 creates the parent directory and skips the write when the on-disk bytes already match.writeNativeCapabilityFixturedoes neither.Two consequences:
- If
plugin/hooks/fixture/does not exist in the target root,writeFileSyncthrowsENOENT.scripts/init.tsline 199 writes into an arbitrary initialized root, so the directory is not guaranteed.- Every
bun run generaterewrites both files and updates their mtimes, even when nothing changed. That defeats mtime-based build caching and makes a no-op generate look like a change.♻️ Proposed change
-import { existsSync, readFileSync, writeFileSync } from "node:fs" -import { join } from "node:path" +import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs" +import { dirname, join } from "node:path"export function writeNativeCapabilityFixture(root: string): GeneratedFile[] { const files = renderNativeCapabilityFixture(root) - for (const file of files) writeFileSync(join(root, file.path), file.contents) + for (const file of files) { + const path = join(root, file.path) + if (existsSync(path) && readFileSync(path, "utf8") === file.contents) continue + mkdirSync(dirname(path), { recursive: true }) + writeFileSync(path, file.contents) + } return files }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/native-capability-fixture.ts` around lines 71 - 75, Update writeNativeCapabilityFixture to match writeGeneratedFiles: create each generated file’s parent directory before writing, and compare existing on-disk bytes so writeFileSync is skipped when contents are unchanged. Preserve returning the full GeneratedFile[] while ensuring new or changed files are written correctly.plugin/skills/capability-tour/references/capability-reviewer.md (1)
38-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive
mutationmore than one legal value.The schema fixes
mutationto the literal"none". A reviewer that cannot confirm the no-mutation constraint has no way to say so, and the caller cannot distinguish a verified claim from a copied constant. SKILL.md line 30 treats the no-mutation report as a precondition for thenative-subagent-via-skilllabel, so the field should be able to fail.♻️ Proposed schema change
- "mutation": "none", + "mutation": "none | unknown",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/skills/capability-tour/references/capability-reviewer.md` at line 38, Update the mutation field in the capability reviewer schema to allow both the verified no-mutation value and a failure/uncertain value, instead of restricting it to the literal "none". Preserve the existing "none" value and align the additional legal value with the report semantics used by SKILL.md and the native-subagent-via-skill precondition.plugin/hooks/native-capability-hook (1)
216-313: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
top_level_stop_guardduplicates about 95 lines oftop_level_string.
whitespace,charat,parse_string, the record accumulation block, and the whole structural scan loop are byte-identical between the two awk programs. Only the key comparison and the value parser differ: lines 291-302 read a JSON boolean instead of a string, and line 309 requiresmatches != 1instead of tolerating zero matches.Two copies means every future fix to the scanner must land twice, and any divergence becomes a silent behavior difference between the
SessionStartandStoppaths.Merge the two into one awk program that takes a mode variable.
♻️ Sketch of the consolidation
-top_level_stop_guard() { - awk ' - function whitespace(c) { ... } - ... - ' "$1" -} +# Scan mode "string" returns a plain top-level string value or __MISSING__. +# Scan mode "boolean" requires exactly one top-level boolean member. +top_level_member() { + awk -v target="$2" -v mode="$3" "$scanner_program" "$1" +}Hold the shared awk source in one shell variable, then branch on
modeat the value-parsing step (lines 130-141 versus lines 291-304) and at the finalmatchescheck (line 145 versus line 309).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/hooks/native-capability-hook` around lines 216 - 313, Consolidate top_level_stop_guard with top_level_string by moving their identical awk scanner logic—whitespace, charat, parse_string, input accumulation, and structural scan—into one shared awk program or shell variable controlled by a mode value. Branch only at value parsing to handle the existing string versus boolean contracts, and preserve each path’s distinct final matches validation, including zero-match tolerance for top_level_string and exactly one match for top_level_stop_guard.scripts/native-capability-hook.test.ts (1)
518-562: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThis test writes to tracked repository files.
Lines 541 and 553 overwrite
plugin/hooks/fixture/lifecycle-mechanics-proof.generated.jsonandlifecycle-mechanics-proof.source.jsonin the working tree. Thefinallyblocks restore them during a normal run. Thefinallyblocks do not run if the test process is killed, for example by a CI job timeout or by Ctrl-C during a watch run. The developer is then left withdrifted\r\nin a tracked file, and the nextgenerate:checkfails for a reason unrelated to the change.Copy the repository into a temporary root, then run
generate:checkthere.generate:checkresolves its own root from itscwd, so the drift assertions still hold.♻️ Proposed approach
-function runGenerateCheck(): ReturnType<typeof Bun.spawnSync> { +function runGenerateCheck(cwd: string = root): ReturnType<typeof Bun.spawnSync> { return Bun.spawnSync({ cmd: [process.execPath, "run", "generate:check"], - cwd: root, + cwd, stdout: "pipe", stderr: "pipe", }) }Then create a temporary root with
cpSyncofplugin.config.json,plugin/,scripts/,package.json, andnode_modules, mutate the fixture files inside that root, and drop bothtry/finallyblocks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/native-capability-hook.test.ts` around lines 518 - 562, Update the test around “generation owns one deterministic LF fixture projection” to operate on a temporary repository copy instead of tracked files. Create a temporary root containing plugin.config.json, plugin/, scripts/, package.json, and node_modules via cpSync, run generate:check with that root as cwd, and mutate the copied fixture files there; remove the existing try/finally restoration blocks while preserving the drift and non-canonical-source assertions.scripts/native-capability-surface.test.ts (2)
13-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParse only the hook declarations instead of every generated file.
Line 15 applies
JSON.parseto the contents of every file returned byrenderGeneratedFiles. The test then reads two entries. If a future generated file is not JSON, this test fails with a parse error that does not describe the hook contract it verifies. Filter to the hook declaration paths before parsing.♻️ Proposed refactor
const generated = new Map( - renderGeneratedFiles(config).map((file) => [file.path, JSON.parse(file.contents)]), + renderGeneratedFiles(config) + .filter((file) => file.path.startsWith("plugin/hooks/")) + .map((file) => [file.path, JSON.parse(file.contents)]), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/native-capability-surface.test.ts` around lines 13 - 16, Update the setup in the test “generation projects one exact native hook declaration per supported client” to filter renderGeneratedFiles(config) to the specific hook declaration paths before calling JSON.parse. Build the generated map only from those filtered files, preserving the existing assertions for the two hook declarations.
204-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReconsider pinning the exact asset digests.
Lines 206-208 hardcode the SHA-256 of both SVG assets. Any byte change to an asset, including a license header or whitespace fix, fails this test with a digest mismatch that does not describe the intent. The maintainer must then recompute and paste a new constant.
The size bound at line 212 and the branding check at line 214 already express the reviewable intent: the assets stay small and identity-neutral. Deterministic packaged bytes are separately enforced by
payloadInventorySha256and the archive proofs inscripts/prove-distribution.ts. Consider dropping the digest constants, or keep them and add a comment that states how to regenerate them.Note also that lines 178-188 restate the
capabilityHookFilesandcapabilityAssetFilesallowlists fromscripts/build.tslines 1628-1635. Importing those constants would keep the test and the validator in step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/native-capability-surface.test.ts` around lines 204 - 216, Remove the hardcoded SHA-256 expectations and hashing assertions from the “identity-neutral assets have deterministic packaged bytes” test, retaining the existing size and branding checks because package determinism is covered elsewhere. Also update the test’s allowlist setup around capabilityHookFiles and capabilityAssetFiles to reuse the exported symbols from build.ts instead of duplicating their values, keeping validation aligned with the build configuration.scripts/build.ts (1)
1628-1650: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the sorted-order requirement of the capability allowlists explicit.
Lines 1702-1709 compare these literals against
inventory.filter(...), whichpluginPayloadInventorysorts withcompareCodeUnits. The comparison therefore requirescapabilityHookFilesandcapabilityAssetFilesto be declared incompareCodeUnitsorder. Both lists satisfy that today. A future entry added out of order fails with a contract-mismatch message that does not explain the real cause. Sort the constants at definition, as line 1713 already does formodelOnlySkillFiles.♻️ Proposed refactor
const capabilityHookFiles = [ "hooks/claude/hooks.json", "hooks/codex/hooks.json", "hooks/fixture/lifecycle-mechanics-proof.generated.json", "hooks/fixture/lifecycle-mechanics-proof.source.json", "hooks/native-capability-hook", -] -const capabilityAssetFiles = ["assets/composer-icon.svg", "assets/logo.svg"] +].sort(compareCodeUnits) +const capabilityAssetFiles = ["assets/composer-icon.svg", "assets/logo.svg"].sort(compareCodeUnits)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build.ts` around lines 1628 - 1650, Sort capabilityHookFiles and capabilityAssetFiles at definition using the existing compareCodeUnits ordering, matching the modelOnlySkillFiles declaration. Preserve the current entries and make the sorted-order contract explicit so future additions remain aligned with pluginPayloadInventory output.scripts/prove-distribution.ts (1)
165-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe payload closure is restated as literals in several files.
scripts/build.tsdeclares the canonical capability allowlists, and the proof script and the surface test each repeat the same skill, hook, and asset lists with their own error messages. Adding or removing one capability file now requires matching edits in every copy, and a missed copy fails late with a message that names the wrong contract. Export the closure constants once fromscripts/build.tsand consume them everywhere.
scripts/prove-distribution.ts#L165-L191: replace the inline["capability-tour","hello-world","runtime-custody","skill-a","skill-b"]and["hello-world","skill-a","skill-b"]literals with the shared exported constants, and reuse them for thehookIndependentSkillslist at lines 269-275.scripts/native-capability-surface.test.ts#L176-L216: importcapabilityHookFilesandcapabilityAssetFilesinstead of restating both inventories at lines 178-188 and the sidecar list at lines 219-226.scripts/build.ts#L1628-L1650: exportcapabilityHookFiles,capabilityAssetFiles, andmodelOnlySkillFiles, and add exported constants for the portable and executable skill identifiers thatscripts/prove-harness-install.tsalso duplicates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/prove-distribution.ts` around lines 165 - 191, Centralize the payload closure definitions in scripts/build.ts by exporting capabilityHookFiles, capabilityAssetFiles, modelOnlySkillFiles, and the portable/executable skill identifier constants used by scripts/prove-harness-install.ts. In scripts/prove-distribution.ts lines 165-191 and 269-275, replace duplicated skill and hook-independent lists with those exports; in scripts/native-capability-surface.test.ts lines 176-216, import and reuse capabilityHookFiles and capabilityAssetFiles for the inventories and sidecar list. Keep each existing validation aligned with the shared constants and retain the relevant contract-specific error messages.scripts/prove-harness-install.ts (2)
1417-1417: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unnecessary escape in the template literal.
Inside a backtick template literal,
\"produces the same"character. The escape suggests the pattern is quoted when it is not.♻️ Proposed cleanup
- if (!launcher.includes(`runtime/runtime-exec\" run ${skillId} --`) || launcher.includes("hooks/")) { + if (!launcher.includes(`runtime/runtime-exec" run ${skillId} --`) || launcher.includes("hooks/")) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/prove-harness-install.ts` at line 1417, Remove the unnecessary backslash before the double quote in the template literal used by the launcher condition near the runtime-exec pattern. Keep the matching text and the existing hooks/ alternative unchanged.
1500-1503: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reporting a failed receipt as
failed, notunknown.When
nativeQualification.statusis"failed", line 1502 setsexternalCandidateQualificationto"unknown". A failed receipt was ingested, so"unknown"understates what the proof knows. The full receipt is still preserved innativeQualification, so no evidence is lost, but a reader ofexternalCandidateQualificationalone cannot distinguish "no receipt" from "receipt failed". Adding a"failed"member to the union would make the three states distinct.This changes the asserted value in
scripts/prove-harness-install.test.ts(line 238), so treat it as a deliberate contract decision rather than a defect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/prove-harness-install.ts` around lines 1500 - 1503, Update externalCandidateQualification in the proof summary to report "failed" when nativeQualification.status is "failed", while retaining "proved" for successful native qualification and "unknown" when no qualification result exists. Extend the associated status union and update the corresponding assertion in the prove harness tests to reflect this deliberate three-state contract.scripts/ship-canary.ts (1)
168-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe error message blames the checksum when
expectedSourceCommitis malformed.Line 168 combines two distinct failures: an invalid
expectedSourceCommitformat, and a checksum that does not match it. Both reportqualification checksum source commit does not match the exact candidate. If the caller passes a malformed expected commit, the message points the operator at the checksum instead of the caller. Splitting the format check would make the cause clear.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ship-canary.ts` around lines 168 - 175, Split the combined guard around expectedSourceCommit into separate validation paths: when it fails the 40-character lowercase hexadecimal format check, throw a CanaryError identifying the expected commit as malformed; otherwise retain the existing checksum-mismatch error only when checksums.sourceCommit differs from expectedSourceCommit.scripts/prove-harness-install.test.ts (1)
377-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific rejection reason for each tampered path.
.toThrow()with no argument passes for any thrown error. Each of the four paths trips a different guard inproveInstalledCapabilityEvidence:hooks/claude/hooks.jsonfails the declaration-bytes check,hooks/fixture/lifecycle-mechanics-proof.generated.jsonfails the fixture-equality check, and the other two reach the payload-hash check. If a future refactor removes one guard, the test still passes because a different guard throws.Assert the expected message per case.
♻️ Proposed refinement
test.each([ - "assets/logo.svg", - "skills/capability-tour/references/capability-reviewer.md", - "hooks/claude/hooks.json", - "hooks/fixture/lifecycle-mechanics-proof.generated.json", -] as const)("installed capability evidence rejects tampered %s bytes", (path) => { + ["assets/logo.svg", "installed payload hash differs"], + ["skills/capability-tour/references/capability-reviewer.md", "installed payload hash differs"], + ["hooks/claude/hooks.json", "installed declaration bytes do not match"], + ["hooks/fixture/lifecycle-mechanics-proof.generated.json", "lifecycle mechanics proof fixture differs"], +] as const)("installed capability evidence rejects tampered %s bytes", (path, reason) => { const temporaryRoot = mkdtempSync(join(tmpdir(), "installed-capability-tamper-")) const pluginRoot = join(temporaryRoot, "plugin") try { cpSync(join(root, "plugin"), pluginRoot, { recursive: true }) const candidatePayloadHash = runtimeClosureEvidence(pluginRoot).payloadHash const absolutePath = join(pluginRoot, path) writeFileSync(absolutePath, Buffer.concat([readFileSync(absolutePath), Buffer.from("tampered\n")])) expect(() => proveInstalledCapabilityEvidence( pluginRoot, "claude", "a".repeat(40), candidatePayloadHash, ), - ).toThrow() + ).toThrow(reason) } finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/prove-harness-install.test.ts` around lines 377 - 401, Update the parameterized test for proveInstalledCapabilityEvidence to associate each tampered path with its expected rejection message, then pass that message to toThrow for path-specific assertions. Expect declaration-bytes rejection for hooks/claude/hooks.json, fixture-equality rejection for hooks/fixture/lifecycle-mechanics-proof.generated.json, and payload-hash rejection for assets/logo.svg and skills/capability-tour/references/capability-reviewer.md.scripts/harness-install-codex.ts (1)
167-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the Codex hook path from one exported constant.
The literal
"./hooks/codex/hooks.json"now appears here, inscripts/prove-dx.ts(line 45), inscripts/release-validate.ts(line 523), and is derived inscripts/prove-harness-install.ts(line 1368).scripts/plugin-config.tsgenerates the same path inhookDeclaration. If the layout changes, four sites must change together. Export one constant (or ahookDeclarationPath(client)helper) fromscripts/plugin-config.tsand reuse it in the non-proof sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/harness-install-codex.ts` around lines 167 - 169, Centralize the Codex hook declaration path in scripts/plugin-config.ts by exporting one shared constant or a hookDeclarationPath(client) helper, then replace the duplicated literal and equivalent derivation in scripts/harness-install-codex.ts, scripts/prove-dx.ts, scripts/release-validate.ts, and other non-proof consumers with that shared symbol. Preserve the existing generated hookDeclaration behavior and exact path value.scripts/release-validate.ts (1)
27-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport every drifted sidecar, not only the first.
Line 23-26 collects all drifted paths, then line 28 reports
drifted[0]. A maintainer fixing drift must rerun validation once per file. List all of them.♻️ Proposed cleanup
if (drifted.length > 0) { - throw new Error(`generated capability sidecar differs from its source: ${drifted[0]}`) + throw new Error(`generated capability sidecars differ from their sources: ${drifted.join(", ")}`) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release-validate.ts` around lines 27 - 29, Update the drift validation check to report every path collected in drifted rather than only drifted[0]. Preserve the existing error behavior while joining or formatting the complete list in the Error message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude-plugin/marketplace.json:
- Line 13: Update claudeMarketplace() to include displayName:
config.displayName, regenerate or edit .claude-plugin/marketplace.json so the
marketplace entry exposes that value, and extend
scripts/plugin-manifest-contract.test.ts to assert the displayName field.
In `@CONTEXT.md`:
- Around line 43-44: Update the Capability Tour description to say it reports
external qualification status, not qualification receipts. Explicitly state that
it never reads, ingests, or infers private qualification receipts, while
preserving the qualification boundary outside the tour and clarifying that its
default result is not an automated native-qualification claim.
In `@docs/plans/native-plugin-capability-tour-plan.md`:
- Around line 243-247: Update the plan’s fail-open behavior to explicitly
require bounded warnings on stderr while keeping stdout empty. Extend
malformed-input, oversized-input, and operational Stop-failure test coverage to
both claude and codex clients, not just claude.
In `@plugin/hooks/native-capability-hook`:
- Around line 52-59: Update both scanner functions containing charat() to detect
the non-portable one-field split fallback: when char_indexed == 0 and
document_length > 65536, exit immediately and fail open before scanning.
Preserve the existing indexed-character path and behavior for documents at or
below the threshold.
In `@scripts/native-capability-hook.test.ts`:
- Around line 418-427: Guard the unreadable-file scenario in the test using a
root-process check, and build the hook-case list so unreadable is omitted when
running as root: [missing, ...(runningAsRoot ? [] : [unreadable]),
noComparison]. Keep the existing chmod setup and assertions unchanged for
non-root environments.
- Line 19: Update the root path initialization near the root constant to use
fileURLToPath on the URL derived from import.meta.url instead of reading
URL.pathname, while preserving the trailing-slash removal so subsequent
join(root, ...) calls receive a decoded filesystem path.
In `@scripts/prove-harness-install.ts`:
- Around line 1700-1706: Update the configuredSourceCommit selection near
resolveCleanSourceCommit and resolveCandidatePayloadCommit to treat empty
SOURCE_COMMIT values as unset, then fall back to GITHUB_SHA or undefined.
Preserve the mismatch validation for non-empty commit values so only an actual
configured commit is compared with sourceCommit.
In `@scripts/release-validate.ts`:
- Around line 11-36: Update validateCapabilitySidecars to verify every path in
STATIC_CAPABILITY_SIDECAR_PATHS exists under repositoryRoot before returning the
inventory, using existsSync and preserving the existing generated-file drift
checks. Throw a validation error identifying the missing sidecar instead of
admitting it as present.
In `@scripts/ship-canary.ts`:
- Around line 1008-1036: Constrain manifestName in installCandidate to the same
kebab-case format enforced by the existing plugin configuration validation
before interpolating it into the deterministicPluginArchive package name. Reject
empty or invalid names with the existing install_mismatch CanaryError, and only
construct the archive path after validation.
---
Nitpick comments:
In `@plugin/hooks/native-capability-hook`:
- Around line 216-313: Consolidate top_level_stop_guard with top_level_string by
moving their identical awk scanner logic—whitespace, charat, parse_string, input
accumulation, and structural scan—into one shared awk program or shell variable
controlled by a mode value. Branch only at value parsing to handle the existing
string versus boolean contracts, and preserve each path’s distinct final matches
validation, including zero-match tolerance for top_level_string and exactly one
match for top_level_stop_guard.
In `@plugin/skills/capability-tour/references/capability-reviewer.md`:
- Line 38: Update the mutation field in the capability reviewer schema to allow
both the verified no-mutation value and a failure/uncertain value, instead of
restricting it to the literal "none". Preserve the existing "none" value and
align the additional legal value with the report semantics used by SKILL.md and
the native-subagent-via-skill precondition.
In `@plugin/skills/capability-tour/SKILL.md`:
- Line 42: Update the “Available portable skills” instruction to require
deriving the reported skills from the installed catalog read earlier, rather
than hardcoding names. Mention hello-world, skill-a, skill-b, runtime-custody,
and capability-tour only as the expected default set, while preserving the
Bun-backed versus model-only classifications.
In `@scripts/build.ts`:
- Around line 1628-1650: Sort capabilityHookFiles and capabilityAssetFiles at
definition using the existing compareCodeUnits ordering, matching the
modelOnlySkillFiles declaration. Preserve the current entries and make the
sorted-order contract explicit so future additions remain aligned with
pluginPayloadInventory output.
In `@scripts/harness-install-codex.ts`:
- Around line 167-169: Centralize the Codex hook declaration path in
scripts/plugin-config.ts by exporting one shared constant or a
hookDeclarationPath(client) helper, then replace the duplicated literal and
equivalent derivation in scripts/harness-install-codex.ts, scripts/prove-dx.ts,
scripts/release-validate.ts, and other non-proof consumers with that shared
symbol. Preserve the existing generated hookDeclaration behavior and exact path
value.
In `@scripts/native-capability-fixture.ts`:
- Around line 71-75: Update writeNativeCapabilityFixture to match
writeGeneratedFiles: create each generated file’s parent directory before
writing, and compare existing on-disk bytes so writeFileSync is skipped when
contents are unchanged. Preserve returning the full GeneratedFile[] while
ensuring new or changed files are written correctly.
In `@scripts/native-capability-hook.test.ts`:
- Around line 518-562: Update the test around “generation owns one deterministic
LF fixture projection” to operate on a temporary repository copy instead of
tracked files. Create a temporary root containing plugin.config.json, plugin/,
scripts/, package.json, and node_modules via cpSync, run generate:check with
that root as cwd, and mutate the copied fixture files there; remove the existing
try/finally restoration blocks while preserving the drift and
non-canonical-source assertions.
In `@scripts/native-capability-surface.test.ts`:
- Around line 13-16: Update the setup in the test “generation projects one exact
native hook declaration per supported client” to filter
renderGeneratedFiles(config) to the specific hook declaration paths before
calling JSON.parse. Build the generated map only from those filtered files,
preserving the existing assertions for the two hook declarations.
- Around line 204-216: Remove the hardcoded SHA-256 expectations and hashing
assertions from the “identity-neutral assets have deterministic packaged bytes”
test, retaining the existing size and branding checks because package
determinism is covered elsewhere. Also update the test’s allowlist setup around
capabilityHookFiles and capabilityAssetFiles to reuse the exported symbols from
build.ts instead of duplicating their values, keeping validation aligned with
the build configuration.
In `@scripts/prove-distribution.ts`:
- Around line 165-191: Centralize the payload closure definitions in
scripts/build.ts by exporting capabilityHookFiles, capabilityAssetFiles,
modelOnlySkillFiles, and the portable/executable skill identifier constants used
by scripts/prove-harness-install.ts. In scripts/prove-distribution.ts lines
165-191 and 269-275, replace duplicated skill and hook-independent lists with
those exports; in scripts/native-capability-surface.test.ts lines 176-216,
import and reuse capabilityHookFiles and capabilityAssetFiles for the
inventories and sidecar list. Keep each existing validation aligned with the
shared constants and retain the relevant contract-specific error messages.
In `@scripts/prove-harness-install.test.ts`:
- Around line 377-401: Update the parameterized test for
proveInstalledCapabilityEvidence to associate each tampered path with its
expected rejection message, then pass that message to toThrow for path-specific
assertions. Expect declaration-bytes rejection for hooks/claude/hooks.json,
fixture-equality rejection for
hooks/fixture/lifecycle-mechanics-proof.generated.json, and payload-hash
rejection for assets/logo.svg and
skills/capability-tour/references/capability-reviewer.md.
In `@scripts/prove-harness-install.ts`:
- Line 1417: Remove the unnecessary backslash before the double quote in the
template literal used by the launcher condition near the runtime-exec pattern.
Keep the matching text and the existing hooks/ alternative unchanged.
- Around line 1500-1503: Update externalCandidateQualification in the proof
summary to report "failed" when nativeQualification.status is "failed", while
retaining "proved" for successful native qualification and "unknown" when no
qualification result exists. Extend the associated status union and update the
corresponding assertion in the prove harness tests to reflect this deliberate
three-state contract.
In `@scripts/release-validate.ts`:
- Around line 27-29: Update the drift validation check to report every path
collected in drifted rather than only drifted[0]. Preserve the existing error
behavior while joining or formatting the complete list in the Error message.
In `@scripts/ship-canary.ts`:
- Around line 168-175: Split the combined guard around expectedSourceCommit into
separate validation paths: when it fails the 40-character lowercase hexadecimal
format check, throw a CanaryError identifying the expected commit as malformed;
otherwise retain the existing checksum-mismatch error only when
checksums.sourceCommit differs from expectedSourceCommit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d7f230ce-ef32-467a-b41b-dacbb0d6e1e7
⛔ Files ignored due to path filters (3)
plugin/assets/composer-icon.svgis excluded by!**/*.svgplugin/assets/logo.svgis excluded by!**/*.svgplugin/hooks/fixture/lifecycle-mechanics-proof.generated.jsonis excluded by!**/*.generated.*
📒 Files selected for processing (43)
.claude-plugin/marketplace.jsonCONTEXT.mdREADME.mddocs/adr/0005-shared-runtime-custody.mddocs/adr/0008-native-plugin-capability-tour.mddocs/plans/native-plugin-capability-tour-plan.mdplugin.config.jsonplugin/.claude-plugin/plugin.jsonplugin/.codex-plugin/plugin.jsonplugin/hooks/claude/hooks.jsonplugin/hooks/codex/hooks.jsonplugin/hooks/fixture/lifecycle-mechanics-proof.source.jsonplugin/hooks/native-capability-hookplugin/skills/capability-tour/SKILL.mdplugin/skills/capability-tour/references/capability-reviewer.mdscripts/build.test.tsscripts/build.tsscripts/dev.test.tsscripts/dev.tsscripts/distribution-checksums.test.tsscripts/distribution-checksums.tsscripts/generate.tsscripts/harness-install-codex.test.tsscripts/harness-install-codex.tsscripts/init.test.tsscripts/init.tsscripts/native-capability-fixture.tsscripts/native-capability-hook.test.tsscripts/native-capability-surface.test.tsscripts/package.tsscripts/plugin-config.tsscripts/plugin-files.tsscripts/plugin-manifest-contract.test.tsscripts/prove-distribution.tsscripts/prove-dx.tsscripts/prove-harness-install.test.tsscripts/prove-harness-install.tsscripts/release-projection.test.tsscripts/release-validate.test.tsscripts/release-validate.tsscripts/runtime-custody-generation.test.tsscripts/ship-canary.test.tsscripts/ship-canary.ts
|
@codex full review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d649319597
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
GitHub Actions
Inactive
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74675e6d7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
scripts/native-capability-surface.test.ts (1)
176-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the capability sidecar lists instead of restating them.
scripts/build.tsdefinescapabilityHookFilesandcapabilityAssetFilesfor the same contract (Lines 1628-1635). This test restates both lists literally, so the contract now has two sources of truth. Export the constants fromscripts/build.tsand assert against them here. The test then still fails when a sidecar is added or removed without updating the payload closure, but it fails for one reason instead of two.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/native-capability-surface.test.ts` around lines 176 - 188, Export the existing capabilityHookFiles and capabilityAssetFiles constants from scripts/build.ts, then import and use them in the payload inventory test instead of duplicating the literal sidecar lists. Preserve the existing filtering and assertions so the test continues validating the payload closure against the shared contract.scripts/prove-harness-install.test.ts (1)
377-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific rejection reason for each tampered path.
.toThrow()with no argument accepts any error. A future refactor that makesproveInstalledCapabilityEvidencefail earlier for an unrelated reason, for example a missing file or a JSON parse error, would keep this test green while the tamper detection is gone. The neighbouring tests at lines 417 and 439 already assert exact messages.Bind each case to its expected failure.
♻️ Proposed refactor
test.each([ - "assets/logo.svg", - "skills/capability-tour/references/capability-reviewer.md", - "hooks/claude/hooks.json", - "hooks/fixture/lifecycle-mechanics-proof.generated.json", -] as const)("installed capability evidence rejects tampered %s bytes", (path) => { + ["assets/logo.svg", "installed payload hash differs"], + ["skills/capability-tour/references/capability-reviewer.md", "installed payload hash differs"], + ["hooks/claude/hooks.json", "installed declaration bytes do not match"], + ["hooks/fixture/lifecycle-mechanics-proof.generated.json", "lifecycle mechanics proof fixture differs"], +] as const)("installed capability evidence rejects tampered %s bytes", (path, reason) => { const temporaryRoot = mkdtempSync(join(tmpdir(), "installed-capability-tamper-")) const pluginRoot = join(temporaryRoot, "plugin") try { cpSync(join(root, "plugin"), pluginRoot, { recursive: true }) const candidatePayloadHash = runtimeClosureEvidence(pluginRoot).payloadHash const absolutePath = join(pluginRoot, path) writeFileSync(absolutePath, Buffer.concat([readFileSync(absolutePath), Buffer.from("tampered\n")])) expect(() => proveInstalledCapabilityEvidence( pluginRoot, "claude", "a".repeat(40), candidatePayloadHash, ), - ).toThrow() + ).toThrow(reason) } finally { rmSync(temporaryRoot, { recursive: true, force: true }) } })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/prove-harness-install.test.ts` around lines 377 - 401, Update the parameterized tampering test around proveInstalledCapabilityEvidence to include the expected rejection message for each path, and pass that message to toThrow. Bind each tampered file case to its specific failure reason, matching the exact-message assertions used by the neighboring tests, so the test verifies tamper detection rather than merely any thrown error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/hooks/codex/hooks.json`:
- Line 8: Update the Codex hook path generation in plugin-config.ts to use the
provided PLUGIN_ROOT variable instead of CODEX_PLUGIN_ROOT, regenerate both
Codex hook commands in hooks.json, and update the generated-manifest assertion
to expect the new variable.
In `@plugin/skills/capability-tour/SKILL.md`:
- Line 8: Update the installed plugin root resolution in the capability-tour
instructions to use three directories above SKILL.md, equivalent to ../../.., so
it resolves plugin rather than plugin/skills and can locate .claude-plugin,
.codex-plugin, and hooks.
In `@scripts/build.ts`:
- Around line 1628-1634: Update the executable-mode validation around the
capability hook file list and payload checks to include
hooks/native-capability-hook alongside runtime/runtime-exec and bin launchers.
Ensure packaging or release validation fails when this hook lacks any executable
bit, preserving the source mode requirement.
In `@scripts/prove-distribution.ts`:
- Around line 225-229: Update the configured commit selection near
resolveCandidatePayloadCommit to use the same empty-tolerant fallback pattern as
prove-harness-install.ts, so an empty SOURCE_COMMIT falls through to GITHUB_SHA
and does not trigger the mismatch check. Preserve the existing comparison and
error behavior for non-empty configured commits.
In `@scripts/prove-harness-install.ts`:
- Around line 1434-1443: Add a 15,000 ms timeout to the Bun.spawnSync options in
runHandler, applying it to both SessionStart and Stop hook invocations while
preserving the existing command, environment, and stream configuration.
---
Nitpick comments:
In `@scripts/native-capability-surface.test.ts`:
- Around line 176-188: Export the existing capabilityHookFiles and
capabilityAssetFiles constants from scripts/build.ts, then import and use them
in the payload inventory test instead of duplicating the literal sidecar lists.
Preserve the existing filtering and assertions so the test continues validating
the payload closure against the shared contract.
In `@scripts/prove-harness-install.test.ts`:
- Around line 377-401: Update the parameterized tampering test around
proveInstalledCapabilityEvidence to include the expected rejection message for
each path, and pass that message to toThrow. Bind each tampered file case to its
specific failure reason, matching the exact-message assertions used by the
neighboring tests, so the test verifies tamper detection rather than merely any
thrown error.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e83dc71-134c-4929-9755-94e744e6a228
⛔ Files ignored due to path filters (3)
plugin/assets/composer-icon.svgis excluded by!**/*.svgplugin/assets/logo.svgis excluded by!**/*.svgplugin/hooks/fixture/lifecycle-mechanics-proof.generated.jsonis excluded by!**/*.generated.*
📒 Files selected for processing (43)
.claude-plugin/marketplace.jsonCONTEXT.mdREADME.mddocs/adr/0005-shared-runtime-custody.mddocs/adr/0008-native-plugin-capability-tour.mddocs/plans/native-plugin-capability-tour-plan.mdplugin.config.jsonplugin/.claude-plugin/plugin.jsonplugin/.codex-plugin/plugin.jsonplugin/hooks/claude/hooks.jsonplugin/hooks/codex/hooks.jsonplugin/hooks/fixture/lifecycle-mechanics-proof.source.jsonplugin/hooks/native-capability-hookplugin/skills/capability-tour/SKILL.mdplugin/skills/capability-tour/references/capability-reviewer.mdscripts/build.test.tsscripts/build.tsscripts/dev.test.tsscripts/dev.tsscripts/distribution-checksums.test.tsscripts/distribution-checksums.tsscripts/generate.tsscripts/harness-install-codex.test.tsscripts/harness-install-codex.tsscripts/init.test.tsscripts/init.tsscripts/native-capability-fixture.tsscripts/native-capability-hook.test.tsscripts/native-capability-surface.test.tsscripts/package.tsscripts/plugin-config.tsscripts/plugin-files.tsscripts/plugin-manifest-contract.test.tsscripts/prove-distribution.tsscripts/prove-dx.tsscripts/prove-harness-install.test.tsscripts/prove-harness-install.tsscripts/release-projection.test.tsscripts/release-validate.test.tsscripts/release-validate.tsscripts/runtime-custody-generation.test.tsscripts/ship-canary.test.tsscripts/ship-canary.ts
GitHub Actions
Inactive
|
@coderabbitai full review |
|
@codex review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2475755ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
GitHub Actions
Inactive
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f9fe7a34e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (15)
scripts/native-capability-fixture.ts (1)
71-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo copies of the fixture write loop skip the directory guard used by
writeGeneratedFiles.writeGeneratedFilesinscripts/plugin-config.tslines 475-484 creates the parent directory and skips a file whose bytes already match. Both fixture write sites do neither, so a root withoutplugin/hooks/fixture/throwsENOENT, and unchanged files are rewritten on every run.
scripts/native-capability-fixture.ts#L71-L75: addmkdirSync(dirname(path), { recursive: true })and the identical-content skip insidewriteNativeCapabilityFixture.scripts/init.ts#L194-L199: remove the duplicatedfor (const file of fixtureFiles) writeFileSync(...)loop and call the shared writer instead, so the guard applies in both paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/native-capability-fixture.ts` around lines 71 - 75, Update scripts/native-capability-fixture.ts:71-75 in writeNativeCapabilityFixture to create each file’s parent directory recursively and skip writing when existing contents match. Update scripts/init.ts:194-199 by removing the duplicated fixture write loop and using the shared writer so both paths apply the same guards.scripts/native-capability-hook.test.ts (1)
533-577: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThis test mutates tracked repository files.
Lines 556 and 568 overwrite
plugin/hooks/fixture/lifecycle-mechanics-proof.generated.jsonand...source.jsoninside the working tree. Thefinallyblocks restore them only on a normal completion. If the runner is interrupted, or anexpectthrows in a way that aborts the process, the checked-in fixture stays drifted and every latergenerate:checkfails.Every other test in this file works inside
mkdtempSyncroots. This test does not.Copy
plugin/andplugin.config.jsoninto a temporary root, then rungenerate:checkwithcwdset to that root. The drift assertions then stay isolated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/native-capability-hook.test.ts` around lines 533 - 577, Update the test generation fixture setup around “generation owns one deterministic LF fixture projection” to create an isolated temporary root with mkdtempSync, copy plugin/ and plugin.config.json into it, and point all source/projection paths and runGenerateCheck calls at that root via cwd. Keep the existing canonical-content and drift assertions, but ensure no tracked repository files are mutated.plugin/hooks/native-capability-hook (1)
262-267: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse
substr()withindex()for the+separatorCommon
awkimplementations treat a single-character separator literally, but POSIX specifiessplit()separators as EREs and does not require this behavior. Usesubstr()withindex()or an escaped ERE such as/\+/to avoid rejecting valid versions such as1.2.3+build.7.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/hooks/native-capability-hook` around lines 262 - 267, Update the +-separator parsing in the identifiers validation block to avoid relying on split(main, build_parts, "+") treating the separator literally; use index() with substr() (or an explicitly escaped ERE) to extract the main version and build metadata, while preserving the existing identifiers validation and rejection behavior.scripts/native-capability-surface.test.ts (2)
112-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInline the single-element loop.
for (const field of ["hooks"] as const)iterates one value. The outer loop adds indirection without adding coverage.♻️ Proposed refactor
- for (const field of ["hooks"] as const) { - for (const manifest of [claudeManifest, codexManifest]) { - expect(manifest[field].startsWith("./")).toBe(true) - expect(existsSync(join(root, "plugin", manifest[field]))).toBe(true) - } - } + for (const manifest of [claudeManifest, codexManifest]) { + expect(manifest.hooks.startsWith("./")).toBe(true) + expect(existsSync(join(root, "plugin", manifest.hooks))).toBe(true) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/native-capability-surface.test.ts` around lines 112 - 117, Remove the single-element outer loop in the test and reference the "hooks" field directly when validating claudeManifest and codexManifest. Preserve both existing assertions for the hooks path and file existence checks.
234-264: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocumentation prose assertions will fail on benign rewrites.
This test requires exact substrings in
README.md,CONTEXT.md, and two ADRs, including"0700","0600", and"macOS and Linux POSIX hosts". Any editorial change to those documents breaks the build even when the underlying behavior is unchanged. If the intent is to prevent overclaiming, keep the negative assertions and the exclusion checks, and reduce the positive prose list to the claims that map to enforced code behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/native-capability-surface.test.ts` around lines 234 - 264, Update the test named “repository docs separate lifecycle mechanics from fresh-native qualification” to remove brittle positive assertions for editorial prose, especially exact wording such as “0700”, “0600”, and “macOS and Linux POSIX hosts”. Retain the negative overclaim checks and capability exclusion assertions, and keep only positive claims directly tied to enforced behavior.scripts/package.ts (1)
72-100: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDerive
archiveBytesfrom the same buffer that produced the digest.
archiveDigestcomes fromarchiveArtifact.bytes, butarchiveBytesre-reads the size from disk. The two values then describe the file through different paths. Use the buffer length so the size and digest always describe the same bytes.♻️ Proposed refactor
-const archiveBytes = statSync(archive).size +const archiveBytes = archiveArtifact.bytes.byteLengthRemove
statSyncfrom thenode:fsimport if it becomes unused.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/package.ts` around lines 72 - 100, Update the archive metadata flow around archiveArtifact and archiveDigest to derive archiveBytes directly from archiveArtifact.bytes.length instead of statSync(archive).size, ensuring the reported size and digest describe the same buffer. Remove statSync from the node:fs import if it is no longer used.scripts/runtime-custody-generation.test.ts (1)
93-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
readdirSyncstatically.The dynamic
(await import("node:fs"))appears at Line 93 and again at Line 133. A static import is clearer and removes the repeated module resolution.♻️ Proposed refactor
Add the static import at the top of the file:
import { readdirSync } from "node:fs"Then simplify both call sites:
- const installedSkills = (await import("node:fs")).readdirSync( - fileURLToPath(new URL("../plugin/skills", import.meta.url)), - ).sort() + const installedSkills = readdirSync( + fileURLToPath(new URL("../plugin/skills", import.meta.url)), + ).sort()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/runtime-custody-generation.test.ts` around lines 93 - 103, Replace the repeated dynamic node:fs imports in the test with a single top-level static readdirSync import, then update both call sites in the relevant test flows to call readdirSync directly while preserving their existing arguments and assertions.scripts/plugin-files.ts (1)
223-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the child stderr in the failure messages.
tarandgzipinherit stderr, so the diagnostic text goes to the parent stream while the thrownErrorcarries only the exit code. In CI the two streams are often separated, which makes packaging failures hard to attribute. Capture stderr and attach it to the error.♻️ Proposed refactor
- const tar = Bun.spawnSync({ cmd: tarArguments, stdout: "inherit", stderr: "inherit" }) + const tar = Bun.spawnSync({ cmd: tarArguments, stdout: "inherit", stderr: "pipe" }) if (tar.exitCode !== 0) { - throw new Error(`deterministic archive tar failed with exit code ${tar.exitCode}`) + throw new Error( + `deterministic archive tar failed with exit code ${tar.exitCode}: ${tar.stderr.toString()}`, + ) } const gzip = Bun.spawnSync({ cmd: ["gzip", "-n", "-9", "-c", uncompressedArchive], stdout: "pipe", - stderr: "inherit", + stderr: "pipe", }) if (gzip.exitCode !== 0) { - throw new Error(`deterministic archive gzip failed with exit code ${gzip.exitCode}`) + throw new Error( + `deterministic archive gzip failed with exit code ${gzip.exitCode}: ${gzip.stderr.toString()}`, + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/plugin-files.ts` around lines 223 - 234, Update the tar and gzip Bun.spawnSync calls in the deterministic archive flow to capture child stderr instead of inheriting it, then include the captured stderr text in each thrown failure Error alongside the exit code. Preserve stdout behavior and ensure both failure messages identify the corresponding command.scripts/release-projection.test.ts (1)
36-56: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe
isReleasePleaseProjection: falseassertion is satisfied by the missing identity.
classifyReleaseImpactsetsisReleasePleaseProjectionto false wheneverpullRequestIdentityis absent, and this call omits it. The assertion therefore passes without exercising the projection path. To prove that a capability sidecar cannot be admitted as a release-please projection, supply a matchingpullRequestIdentitywithexpectedAutomationLogin,authorLogin, and arelease-please--branches--head ref, and keep the same expectation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release-projection.test.ts` around lines 36 - 56, Update the classifyReleaseImpact call in the static capability sidecars test to include a matching pullRequestIdentity with expectedAutomationLogin, authorLogin, and a release-please--branches-- head ref. Keep the existing payloadChanged, isReleasePleaseProjection, and ok expectations so the assertion exercises the projection path rather than the missing-identity fallback.scripts/build.ts (1)
1628-1650: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort the hook and asset allowlists at comparison time.
capabilityHookFilesandcapabilityAssetFilesare compared against the sorted inventory without sorting (Lines 1702-1709), so the literal order must matchcompareCodeUnitsexactly. The skill comparison already sorts (Line 1713). If a maintainer adds an entry in the wrong position, validation fails with an inventory-mismatch error that does not point at the authoring mistake. Sort both lists once at module scope.♻️ Proposed refactor
-const capabilityHookFiles = [ +const capabilityHookFiles = [ "hooks/claude/hooks.json", "hooks/codex/hooks.json", "hooks/fixture/lifecycle-mechanics-proof.generated.json", "hooks/fixture/lifecycle-mechanics-proof.source.json", "hooks/native-capability-hook", -] -const capabilityAssetFiles = ["assets/composer-icon.svg", "assets/logo.svg"] +].sort(compareCodeUnits) +const capabilityAssetFiles = ["assets/composer-icon.svg", "assets/logo.svg"].sort(compareCodeUnits)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build.ts` around lines 1628 - 1650, Sort capabilityHookFiles and capabilityAssetFiles at module scope using the existing compareCodeUnits ordering, so their literal order matches the sorted inventory used by the comparisons near the hook and asset validation flow. Keep the existing skill-list sorting unchanged and ensure both allowlists are normalized once rather than relying on maintainers to preserve manual order.scripts/build.test.ts (1)
1576-1586: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten the expected rejection message per path.
The regex
/Bun payload closure|unsafe plugin payload entry/matches any payload-closure failure. A future regression that fails for an unrelated reason still passes this test. Removingskills/capability-tour/references/capability-reviewer.mdempties that directory, sopluginPayloadInventoryreports an empty directory, while the other two paths reportBun payload closure: missing <path>. Map each path to its exact expected message.♻️ Proposed refactor
-test.each([ - "hooks/native-capability-hook", - "assets/logo.svg", - "skills/capability-tour/references/capability-reviewer.md", -] as const)("Bun-only release admission rejects missing capability sidecar %s", (path) => { - const fixtureRoot = bunPayloadFixture() - rmSync(join(fixtureRoot, "plugin", path)) - expect(() => validateBunOnlyPayload(fixtureRoot)).toThrow( - /Bun payload closure|unsafe plugin payload entry/, - ) -}) +test.each([ + ["hooks/native-capability-hook", "Bun payload closure: missing hooks/native-capability-hook"], + ["assets/logo.svg", "Bun payload closure: missing assets/logo.svg"], + [ + "skills/capability-tour/references/capability-reviewer.md", + "unsafe plugin payload entry", + ], +] as const)("Bun-only release admission rejects missing capability sidecar %s", (path, message) => { + const fixtureRoot = bunPayloadFixture() + rmSync(join(fixtureRoot, "plugin", path)) + expect(() => validateBunOnlyPayload(fixtureRoot)).toThrow(message) +})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build.test.ts` around lines 1576 - 1586, Update the parameterized test around validateBunOnlyPayload to associate each removed path with its exact expected rejection message: use the empty-directory message for skills/capability-tour/references/capability-reviewer.md and the missing-path Bun payload closure message for the other fixtures. Assert the mapped message directly instead of using the broad alternation regex.scripts/release-validate.ts (1)
23-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport every drifted sidecar, not only the first.
Line 28 emits
drifted[0]. When several sidecars drift, the maintainer must rerun validation for each one. The static-sidecar loop at Lines 30-34 has the same shape.♻️ Proposed change
if (drifted.length > 0) { - throw new Error(`generated capability sidecar differs from its source: ${drifted[0]}`) + throw new Error(`generated capability sidecars differ from their source: ${drifted.join(", ")}`) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release-validate.ts` around lines 23 - 29, Update the drift error reporting in the validation flow around checkGeneratedFiles, checkNativeCapabilityFixture, and the static-sidecar loop to include every detected drifted sidecar rather than only drifted[0]. Preserve the existing failure behavior while aggregating all drift paths in the emitted error message.scripts/prove-harness-install.ts (2)
1434-1471: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winVerify the payload hash before you execute the installed hook.
runHandlerspawnshooks/native-capability-hookfrompluginRootat Lines 1445 and 1464. The payload-hash comparison runs afterwards at Line 1469. When the candidate payload is a remote checkout (installCandidateinscripts/ship-canary.tsLines 976-1047), the harness executes candidate-controlled bytes before it confirms those bytes match the expected candidate hash.Move the
runtimeClosureEvidencecomparison above the handler execution. The check is cheap and it closes the window.🛡️ Proposed reordering
if (typeof manifest.version !== "string") throw new Error(`${client} installed version is invalid`) + const installed = runtimeClosureEvidence(pluginRoot, installedInventory) + if (installed.payloadHash !== candidatePayloadHash) { + throw new Error(`${client} installed payload hash differs from the candidate payload`) + } const handler = join(pluginRoot, "hooks", "native-capability-hook") @@ if (stop.exitCode !== 0 || stop.stdout.toString() !== "" || stop.stderr.toString() !== "") { throw new Error(`${client} installed direct Stop handler check failed`) } - const installed = runtimeClosureEvidence(pluginRoot, installedInventory) - if (installed.payloadHash !== candidatePayloadHash) { - throw new Error(`${client} installed payload hash differs from the candidate payload`) - }Note: the tamper test at
scripts/prove-harness-install.test.tsLines 377-401 still passes after this change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/prove-harness-install.ts` around lines 1434 - 1471, Move the runtimeClosureEvidence payload-hash validation before the first runHandler invocation in the installed-hook verification flow. Compute installed evidence and compare installed.payloadHash with candidatePayloadHash immediately after defining runHandler, throwing the existing mismatch error before executing either SessionStart or Stop; remove the later duplicate comparison while preserving the existing handler checks.
1646-1669: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd a timeout to the two Git subprocess calls.
commandat Line 512 usestimeout: 15_000, andrunHandlerat Line 1440 now does the same.resolveCleanPathsCommitspawnsgit statusandgit rev-parsewith no timeout. If either call hangs, the proof blocks with no bound.♻️ Proposed change
const status = Bun.spawnSync({ cmd: ["git", "status", "--porcelain=v1", "--untracked-files=all", "--", ...paths], cwd: repositoryRoot, + timeout: 15_000, stdout: "pipe", stderr: "pipe", }) @@ const head = Bun.spawnSync({ cmd: ["git", "rev-parse", "HEAD"], cwd: repositoryRoot, + timeout: 15_000, stdout: "pipe", stderr: "pipe", })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/prove-harness-install.ts` around lines 1646 - 1669, Update both Bun.spawnSync calls in resolveCleanPathsCommit, for git status and git rev-parse, to include the existing 15,000 millisecond timeout used by command and runHandler. Preserve the current arguments, exit handling, and validation behavior.scripts/prove-harness-install.test.ts (1)
377-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific rejection reason in the tamper test.
Line 390 uses a bare
.toThrow(). Any error satisfies it, including an unrelated spawn or parse failure. Forassets/logo.svgthe only guard is the payload-hash comparison atscripts/prove-harness-install.tsLine 1469. Pin the expected message so the test proves that guard fired.♻️ Proposed change
-] as const)("installed capability evidence rejects tampered %s bytes", (path) => { +] as const)("installed capability evidence rejects tampered %s bytes", (path) => { const temporaryRoot = mkdtempSync(join(tmpdir(), "installed-capability-tamper-")) const pluginRoot = join(temporaryRoot, "plugin") try { cpSync(join(root, "plugin"), pluginRoot, { recursive: true }) const candidatePayloadHash = runtimeClosureEvidence(pluginRoot).payloadHash const absolutePath = join(pluginRoot, path) writeFileSync(absolutePath, Buffer.concat([readFileSync(absolutePath), Buffer.from("tampered\n")])) expect(() => proveInstalledCapabilityEvidence( pluginRoot, "claude", "a".repeat(40), candidatePayloadHash, ), - ).toThrow() + ).toThrow(/installed (payload hash differs|declaration bytes|lifecycle mechanics proof)/) } finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/prove-harness-install.test.ts` around lines 377 - 401, Update the tampered-bytes test around proveInstalledCapabilityEvidence to assert the specific payload-hash mismatch rejection message rather than using a bare toThrow(). Ensure the assertion verifies the guard in proveInstalledCapabilityEvidence rejects the modified assets/logo.svg and other tampered files because their computed payload hash no longer matches the candidate hash.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/plans/native-plugin-capability-tour-plan.md`:
- Line 360: Update the acceptance-gate statement in the native plugin capability
tour plan to distinguish the identifiers: require the source commit, archive
checksum, packaged payload hash, and installed payload hash to bind to the same
candidate lineage, while requiring equality only between the packaged and
installed payload hashes.
In `@plugin/skills/capability-tour/SKILL.md`:
- Line 17: Update the direct-handler invocation contract in
plugin/skills/capability-tour/SKILL.md:17-17 and check 4 in
plugin/skills/capability-tour/references/capability-reviewer.md:21-21 to require
the absolute path <installed plugin root>/hooks/native-capability-hook, followed
by exactly the arguments <event> <client>—for this tour, Stop and the active
client. Preserve the existing stdin payload and result-reporting behavior.
In `@scripts/build.ts`:
- Around line 1773-1788: Add "commands" to the forbidden component list used by
the manifest validation loop in the build script, so unsupported top-level
commands declarations trigger the existing Bun payload closure error. Preserve
the current rejection behavior and error format for all other forbidden
components.
In `@scripts/plugin-files.ts`:
- Around line 157-236: Update deterministicPluginArchive to produce identical
USTAR headers on Linux and macOS by using one canonical archive implementation
or explicitly setting matching uname and gname values in both tar argument
paths. Preserve the existing deterministic metadata and gzip flow, and add a
cross-platform digest fixture test before relying on archiveSha256.
In `@scripts/prove-distribution.ts`:
- Around line 165-191: Update the runtime-closure validation around
catalog.skills and bundles.bundles to include the actual sorted surface keys in
the error message when they differ from the expected executable closure.
Preserve the existing comparison and capability-tour context, and report the
computed keys for the failing surface.
In `@scripts/release-validate.test.ts`:
- Around line 219-231: Update the parameterized test “release validation rejects
stale generated capability bytes at %s” to clean up each copyRepository() result
with rmSync in a try/finally block, matching the existing cleanup pattern used
by the nearby test while preserving the validation assertions.
In `@scripts/ship-canary.ts`:
- Around line 1033-1047: Validate manifestVersion as a bounded semantic version
before the deterministicPluginArchive call in the lineage construction,
rejecting traversal or path-like values while preserving Claude/Codex version
agreement checks. Reuse the existing semantic-version validation utility if
available, and ensure invalid versions cannot reach archive staging or payload
copying.
---
Nitpick comments:
In `@plugin/hooks/native-capability-hook`:
- Around line 262-267: Update the +-separator parsing in the identifiers
validation block to avoid relying on split(main, build_parts, "+") treating the
separator literally; use index() with substr() (or an explicitly escaped ERE) to
extract the main version and build metadata, while preserving the existing
identifiers validation and rejection behavior.
In `@scripts/build.test.ts`:
- Around line 1576-1586: Update the parameterized test around
validateBunOnlyPayload to associate each removed path with its exact expected
rejection message: use the empty-directory message for
skills/capability-tour/references/capability-reviewer.md and the missing-path
Bun payload closure message for the other fixtures. Assert the mapped message
directly instead of using the broad alternation regex.
In `@scripts/build.ts`:
- Around line 1628-1650: Sort capabilityHookFiles and capabilityAssetFiles at
module scope using the existing compareCodeUnits ordering, so their literal
order matches the sorted inventory used by the comparisons near the hook and
asset validation flow. Keep the existing skill-list sorting unchanged and ensure
both allowlists are normalized once rather than relying on maintainers to
preserve manual order.
In `@scripts/native-capability-fixture.ts`:
- Around line 71-75: Update scripts/native-capability-fixture.ts:71-75 in
writeNativeCapabilityFixture to create each file’s parent directory recursively
and skip writing when existing contents match. Update scripts/init.ts:194-199 by
removing the duplicated fixture write loop and using the shared writer so both
paths apply the same guards.
In `@scripts/native-capability-hook.test.ts`:
- Around line 533-577: Update the test generation fixture setup around
“generation owns one deterministic LF fixture projection” to create an isolated
temporary root with mkdtempSync, copy plugin/ and plugin.config.json into it,
and point all source/projection paths and runGenerateCheck calls at that root
via cwd. Keep the existing canonical-content and drift assertions, but ensure no
tracked repository files are mutated.
In `@scripts/native-capability-surface.test.ts`:
- Around line 112-117: Remove the single-element outer loop in the test and
reference the "hooks" field directly when validating claudeManifest and
codexManifest. Preserve both existing assertions for the hooks path and file
existence checks.
- Around line 234-264: Update the test named “repository docs separate lifecycle
mechanics from fresh-native qualification” to remove brittle positive assertions
for editorial prose, especially exact wording such as “0700”, “0600”, and “macOS
and Linux POSIX hosts”. Retain the negative overclaim checks and capability
exclusion assertions, and keep only positive claims directly tied to enforced
behavior.
In `@scripts/package.ts`:
- Around line 72-100: Update the archive metadata flow around archiveArtifact
and archiveDigest to derive archiveBytes directly from
archiveArtifact.bytes.length instead of statSync(archive).size, ensuring the
reported size and digest describe the same buffer. Remove statSync from the
node:fs import if it is no longer used.
In `@scripts/plugin-files.ts`:
- Around line 223-234: Update the tar and gzip Bun.spawnSync calls in the
deterministic archive flow to capture child stderr instead of inheriting it,
then include the captured stderr text in each thrown failure Error alongside the
exit code. Preserve stdout behavior and ensure both failure messages identify
the corresponding command.
In `@scripts/prove-harness-install.test.ts`:
- Around line 377-401: Update the tampered-bytes test around
proveInstalledCapabilityEvidence to assert the specific payload-hash mismatch
rejection message rather than using a bare toThrow(). Ensure the assertion
verifies the guard in proveInstalledCapabilityEvidence rejects the modified
assets/logo.svg and other tampered files because their computed payload hash no
longer matches the candidate hash.
In `@scripts/prove-harness-install.ts`:
- Around line 1434-1471: Move the runtimeClosureEvidence payload-hash validation
before the first runHandler invocation in the installed-hook verification flow.
Compute installed evidence and compare installed.payloadHash with
candidatePayloadHash immediately after defining runHandler, throwing the
existing mismatch error before executing either SessionStart or Stop; remove the
later duplicate comparison while preserving the existing handler checks.
- Around line 1646-1669: Update both Bun.spawnSync calls in
resolveCleanPathsCommit, for git status and git rev-parse, to include the
existing 15,000 millisecond timeout used by command and runHandler. Preserve the
current arguments, exit handling, and validation behavior.
In `@scripts/release-projection.test.ts`:
- Around line 36-56: Update the classifyReleaseImpact call in the static
capability sidecars test to include a matching pullRequestIdentity with
expectedAutomationLogin, authorLogin, and a release-please--branches-- head ref.
Keep the existing payloadChanged, isReleasePleaseProjection, and ok expectations
so the assertion exercises the projection path rather than the missing-identity
fallback.
In `@scripts/release-validate.ts`:
- Around line 23-29: Update the drift error reporting in the validation flow
around checkGeneratedFiles, checkNativeCapabilityFixture, and the static-sidecar
loop to include every detected drifted sidecar rather than only drifted[0].
Preserve the existing failure behavior while aggregating all drift paths in the
emitted error message.
In `@scripts/runtime-custody-generation.test.ts`:
- Around line 93-103: Replace the repeated dynamic node:fs imports in the test
with a single top-level static readdirSync import, then update both call sites
in the relevant test flows to call readdirSync directly while preserving their
existing arguments and assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c949b956-8f5b-494a-a778-c8dca4ef343a
⛔ Files ignored due to path filters (3)
plugin/assets/composer-icon.svgis excluded by!**/*.svgplugin/assets/logo.svgis excluded by!**/*.svgplugin/hooks/fixture/lifecycle-mechanics-proof.generated.jsonis excluded by!**/*.generated.*
📒 Files selected for processing (43)
.claude-plugin/marketplace.jsonCONTEXT.mdREADME.mddocs/adr/0005-shared-runtime-custody.mddocs/adr/0008-native-plugin-capability-tour.mddocs/plans/native-plugin-capability-tour-plan.mdplugin.config.jsonplugin/.claude-plugin/plugin.jsonplugin/.codex-plugin/plugin.jsonplugin/hooks/claude/hooks.jsonplugin/hooks/codex/hooks.jsonplugin/hooks/fixture/lifecycle-mechanics-proof.source.jsonplugin/hooks/native-capability-hookplugin/skills/capability-tour/SKILL.mdplugin/skills/capability-tour/references/capability-reviewer.mdscripts/build.test.tsscripts/build.tsscripts/dev.test.tsscripts/dev.tsscripts/distribution-checksums.test.tsscripts/distribution-checksums.tsscripts/generate.tsscripts/harness-install-codex.test.tsscripts/harness-install-codex.tsscripts/init.test.tsscripts/init.tsscripts/native-capability-fixture.tsscripts/native-capability-hook.test.tsscripts/native-capability-surface.test.tsscripts/package.tsscripts/plugin-config.tsscripts/plugin-files.tsscripts/plugin-manifest-contract.test.tsscripts/prove-distribution.tsscripts/prove-dx.tsscripts/prove-harness-install.test.tsscripts/prove-harness-install.tsscripts/release-projection.test.tsscripts/release-validate.test.tsscripts/release-validate.tsscripts/runtime-custody-generation.test.tsscripts/ship-canary.test.tsscripts/ship-canary.ts
GitHub Actions
Inactive
Addressed across 1328f86 and 185355d as one bounded final batch:
Full exact-head proof is green: 580 core tests, 61 runtime-custody tests, and native install, distribution, and DX proofs. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/ship-canary.ts (2)
1035-1037: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winCheck the manifest name length before the regex.
manifestNameis read from the candidate checkout. Line 1036 evaluates the regex before Line 1037 enforces the 64-character bound. Move the length check before the regex, asvalidateLineageManifestVersionalready does.Proposed fix
if ( typeof manifestName !== "string" || - !/^[a-z0-9]+(?:-[a-z0-9]+)*$/.test(manifestName) || - manifestName.length > 64 + manifestName.length > 64 || + !/^[a-z0-9]+(?:-[a-z0-9]+)*$/.test(manifestName) ) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ship-canary.ts` around lines 1035 - 1037, In the manifest-name validation condition, move the manifestName.length > 64 check before the regular-expression test, matching the ordering used by validateLineageManifestVersion. Preserve the existing type check and validation behavior.
1058-1061: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBind lineage to the published archive checksum.
deterministicPluginArchive(checkoutRoot, ...)computes a local rebuild digest. It does not prove that the hosted*.checksums.jsonvalue matches. Load the sidecar for the exact candidate, compare itsarchiveSha256with the rebuilt digest, and reject mismatches beforebindCandidateQualificationLineage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ship-canary.ts` around lines 1058 - 1061, Before bindCandidateQualificationLineage, load the *.checksums.json sidecar for the exact candidate and compare its archiveSha256 with the digest returned by deterministicPluginArchive(checkoutRoot, ...). Reject the candidate on any mismatch, and only pass the verified hosted checksum into the lineage binding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/ship-canary.ts`:
- Around line 1035-1037: In the manifest-name validation condition, move the
manifestName.length > 64 check before the regular-expression test, matching the
ordering used by validateLineageManifestVersion. Preserve the existing type
check and validation behavior.
- Around line 1058-1061: Before bindCandidateQualificationLineage, load the
*.checksums.json sidecar for the exact candidate and compare its archiveSha256
with the digest returned by deterministicPluginArchive(checkoutRoot, ...).
Reject the candidate on any mismatch, and only pass the verified hosted checksum
into the lineage binding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d5dc5bf-56b9-416c-8008-7820fe78fbfa
📒 Files selected for processing (23)
docs/plans/native-plugin-capability-tour-plan.mdplugin/hooks/native-capability-hookplugin/skills/capability-tour/SKILL.mdplugin/skills/capability-tour/references/capability-reviewer.mdscripts/build.test.tsscripts/build.tsscripts/init.tsscripts/native-capability-fixture.tsscripts/native-capability-hook.test.tsscripts/native-capability-surface.test.tsscripts/package.tsscripts/plugin-config.tsscripts/plugin-files.test.tsscripts/plugin-files.tsscripts/prove-distribution.tsscripts/prove-harness-install.test.tsscripts/prove-harness-install.tsscripts/release-projection.test.tsscripts/release-validate.test.tsscripts/release-validate.tsscripts/runtime-custody-generation.test.tsscripts/ship-canary.test.tsscripts/ship-canary.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- plugin/skills/capability-tour/references/capability-reviewer.md
- scripts/release-projection.test.ts
- plugin/skills/capability-tour/SKILL.md
- scripts/init.ts
- scripts/prove-distribution.ts
- scripts/release-validate.test.ts
- scripts/plugin-files.ts
- scripts/runtime-custody-generation.test.ts
- scripts/release-validate.ts
- scripts/package.ts
- scripts/native-capability-surface.test.ts
- scripts/build.ts
- scripts/native-capability-hook.test.ts
- plugin/hooks/native-capability-hook
- docs/plans/native-plugin-capability-tour-plan.md
- scripts/plugin-config.ts
- scripts/native-capability-fixture.ts
- scripts/prove-harness-install.ts
- scripts/build.test.ts
- scripts/prove-harness-install.test.ts
GitHub Actions
Inactive
|
@codex review |
|
Resolved the two body-only notes from late CodeRabbit review 4893529244 against current head 185355d:
All inline review threads are resolved. Exact-head local proof: 580 pass, 3 skip; runtime custody 61 pass; native install, distribution, and DX green. Exact-head hosted checks are green; Codex rereview is running. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 185355dcd2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
GitHub Actions
Inactive
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex-gate approve a40517a 5235699817 |
|
To use Codex here, create an environment for this repo. |
|
@codex-gate approve a40517a 5235699817 |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…rift The tree enumerates every skill and launcher by hand with nothing generating those lines, which is how capability-tour stayed missing between #34 and 24102bc. Derives both expected lists from plugin/skills/ and plugin/bin/. Closes the same defect class the capability-tour inventory guard covers in e4c6ea4; the README half was left open there. Sensitivity proven both ways: planting a sixth skill directory turns it RED on the skills line, planting a fourth launcher turns it RED on the bin line, and removing each returns GREEN.
…oc (#42) * docs: list capability-tour in the architecture tree The "Architecture at a glance" tree enumerated four skills; plugin/skills/ has five. capability-tour shipped in #34 without updating the diagram. The line is an exhaustive list, not shorthand: e927c24 (#22) expanded it from one entry to four when skills were added, and the sibling bin/ line enumerates all three files in plugin/bin/. The same README already depends on capability-tour as an existing feature further down. * docs: list prove:harness-install and guard the skill inventory Two findings from a full doc-surface audit. README's proof-command list omitted `prove:harness-install`, the only prove:* script missing from it. It runs inside `prove:all` and is the named authority in docs/native-capability-qualification.md for package, declaration, and installed-byte proof. capability-tour/SKILL.md enumerates the portable skills in prose with no generation step behind it, so a new skill silently falsifies the line. That is the same drift that left capability-tour out of README's architecture tree until 24102bc. Adds a test deriving the expected inventory from plugin/skills/ and runtime/skill-catalog.json. Sensitivity proven: planting a sixth skill directory turns the guard RED on the missing name; removing it returns GREEN. * docs: declare what verifies each document Maps each doc to the artifact that settles its claims, so a docs audit reads release.yml when checking publishing.md instead of hoping a scanner finds it. Four docs specify workflow behaviour in roughly 130 assertions between them; without the map those go unread. Also declares what this repo cannot check about itself. installing.md is ~90 claims about external Claude and Codex CLI surface, native-capability- qualification.md rests on human receipts, canary-qualification.md needs a hosted run. Scanning those returns nothing whether they are accurate or wrong, so they are marked unverifiable and reported rather than silently passing. Superseded ADRs 0002 and 0004 and docs/plans/ are frozen - they record history correctly. Consumed by the docs-drift skill. Every path verified to resolve. * test: guard the README architecture tree against skill and launcher drift The tree enumerates every skill and launcher by hand with nothing generating those lines, which is how capability-tour stayed missing between #34 and 24102bc. Derives both expected lists from plugin/skills/ and plugin/bin/. Closes the same defect class the capability-tour inventory guard covers in e4c6ea4; the README half was left open there. Sensitivity proven both ways: planting a sixth skill directory turns it RED on the skills line, planting a fourth launcher turns it RED on the bin line, and removing each returns GREEN. * Address PR review feedback (#42) - Parse the skill inventory instead of probing for each name. The containment loop passed while the prose named a skill that does not ship, and passed with a model-only skill inside the Bun-backed prefix; both reproduced before the change and both now fail. Also asserts every catalog entry ships, which is drift in the other direction. - Add plugin/skills/capability-tour/SKILL.md and scripts/prove-harness-install.ts to the README manifest entry. The architecture tree claims capability-tour ships and the proof list describes what prove:harness-install checks; neither package.json nor runtime.lock.json can settle those. --------- Co-authored-by: Nathan Vale <hi@nathanvale.com>
This branch was previously deployed
2 participants
Summary
The installed plugin is now a guided capability tour instead of a generic skill bundle. An evaluator can install one plugin in Claude Code or Codex, decide hook trust, and run the tour to learn which portable plugin capabilities actually work in their client — with every claim labeled by the evidence that backs it. Nothing from v0.2.0 regresses: all four portable skills, the three Bun launchers, and runtime custody ship unchanged.
Implements docs/plans/native-plugin-capability-tour-plan.md (U1-U4). Session-settled decisions carried from planning: one capability tour extending the same plugin (user-directed, over a second proof plugin); agentless delegation via a skill-local reviewer prompt (user-approved, over a packaged agent or Codex companion); no MCP in v0.3.0 (user-approved); plugin-owned drift fixture (user-directed, over executing workspace checks); visible success once at session start (user-directed).
Design decisions worth review attention:
stop_hook_activewith a narrow awk lexical scan (linear-time, 1 MiB cap) — deliberately not a JSON parser. String/nested decoys, duplicate keys, and oversized input are all covered by tests.currentSessionHookderives only from an explicit current-session marker; external qualification receipts cannot flip it. Direct handler execution never satisfies a native-activation field.Validation
Full Bun suite: 630 pass / 0 fail (three consecutive runs at the final head).
generate:check,build,package,prove:distribution,prove:runtime-custody,prove:runtime-platform(darwin-arm64),release:validate --json, andprove:allall green;git diff --checkclean. Hook behavior verified against isolated temp plugin roots with byte-level tree snapshots (no-mutation proof), hostile cwd/PATH, and a timing regression bound on ~1 MiB hostile payloads. Multi-lens code review (correctness, security, testing, maintainability, agent-native, plus an independent cross-model adversarial pass) returned five actionable findings, all applied and re-verified in d649319 — including an empirically reproduced payload-closure gap and a quadratic awk scan measured at ~70s/MiB, now 0.5-1.1s.Known Residuals
stop_hook_activefrom JSON that is malformed elsewhere (e.g. a missing comma between other members), so a real fixture mismatch still blocks on such input. Broader top-level grammar validation would fix it but tensions with the plan's no-general-JSON-parser decision.plugin/hooks/native-capability-hook:264.Post-Deploy Monitoring & Validation
No production runtime impact — this is a template repository. Validation is hosted CI on this PR (must be green) plus the human-operated fresh-client qualification documented in README/ADR 0008 before any v0.3.0 release tag. Failure signals: red hosted checks, or a qualification receipt whose lineage hashes do not match the candidate. Mitigation: fix on this branch before merge; no rollback surface exists.
New concepts
Fail-open lifecycle drift proof. A plugin ships two copies of one small fixture — a canonical source and a projection generated at build time — and its Stop hook compares their exact bytes at session end. A mismatch is the only condition that blocks (once, with a fixed reason); every operational ambiguity — malformed input, missing fixture, unreadable version, absent tooling — exits successfully with at most one bounded warning. Chosen over asserting workspace state because an automatic hook must never execute or judge user project code: the plugin can only make honest claims about bytes it shipped. This PR exercises it end to end: generation owns the projection (
scripts/native-capability-fixture.ts), the handler only compares (cmpon shipped bytes), and re-entry withstop_hook_active: trueis silent so one Stop attempt never loops. The boundary: this proves lifecycle mechanics, not integrity or security — don't reach for it as a tamper-detection or workspace-quality gate.Summary by CodeRabbit