feat(intent): add reviewable skill provenance with lockfiles and frozen CI#199
feat(intent): add reviewable skill provenance with lockfiles and frozen CI#199LadyBluenotes wants to merge 66 commits into
Conversation
…proved source resolution
- Introduced 'skills' field in IntentLockfileSource and related types. - Updated lockfile diffing logic to compare skills. - Refactored source content hashing to handle skills independently. - Enhanced tests to cover new skills functionality and ensure backward compatibility. - Adjusted error handling and exit codes for various commands in frozen mode.
… CI auto-detection
…est` commands for intent skills
…omizable filesystem support
…ance workspace integration in scanner
…kage kind support
…es outside the consumer repository; add tests for frozen mode and approved installed dependencies
… and update related tests
- Updated `intent-skills.md` to include `manifestHash` and `capabilities` in the mechanical refresh description. - Improved `lockfile.md` to clarify the conditions under which `manifestHash` and `capabilities` are populated. - Refactored `intent-core.ts` to streamline imports and maintain consistency. - Enhanced `hash.ts` to enforce limits on entry counts and added validation for hash entries. - Updated `lockfile-state.ts` to assert manifest matches package details. - Improved `lockfile.ts` to reject undeclared fields in lockfile parsing. - Enhanced `manifest.ts` to validate capabilities and MCP tools against declared fields. - Added tests to ensure proper handling of unknown capabilities and undeclared fields in manifests. - Updated resolver logic to handle skill resolution more effectively, especially for same-name skills across different sources.
…ring changes; update CLI options and tests
|
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:
📝 WalkthroughWalkthroughAdds ChangesIntent lockfile and skills workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Scanner
participant Lockfile
participant Approval
participant Loader
CLI->>Scanner: scan installed skill sources
Scanner->>Lockfile: compute current identities and hashes
Lockfile-->>CLI: report missing, clean, or changed state
CLI->>Approval: approve or update selected sources
Approval->>Lockfile: write canonical intent.lock
Loader->>Lockfile: validate approved state before loading
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 7e7adb5
☁️ Nx Cloud last updated this comment at |
commit: |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/intent/src/shared/utils.ts (1)
252-269: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass the resolved frozen state into scan options
scanOptionsFromGlobalFlags()only setsscope, soscanForIntents()still reachesdetectGlobalNodeModules(..., options.frozen)withundefined. That lets CI auto-detect win even when--no-frozenwas passed.🤖 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 `@packages/intent/src/shared/utils.ts` around lines 252 - 269, Update scanOptionsFromGlobalFlags() to resolve and include the effective frozen state in its returned options, preserving the explicit --no-frozen value instead of leaving frozen undefined. Ensure scanForIntents() passes this resolved options.frozen to detectGlobalNodeModules() so caller flags take precedence over CI auto-detection.
🧹 Nitpick comments (1)
docs/security/lockfile.md (1)
62-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider varying the sentence structure in the frozen-mode failure list.
Four consecutive bullet points begin with "Fails." While the parallel structure is readable as a reference list, varying the phrasing would improve flow.
🤖 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 `@docs/security/lockfile.md` around lines 62 - 65, Vary the sentence openings in the frozen-mode failure list while preserving the existing meanings, exit codes, and commands; update the bullets describing drift, unapproved skill-bearing sources, a missing intent.lock, and malformed or unsupported intent.lock files.Source: Linters/SAST tools
🤖 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 `@packages/intent/src/skills/resolver.ts`:
- Around line 166-169: Map.groupBy in the resolver is unsupported on the
declared Node 20 minimum runtime. Replace the grouping in the packagesByIdentity
initialization with a Node 20-compatible approach, or consistently raise the
package and README Node version requirements to a runtime that supports
Map.groupBy.
In `@packages/intent/src/staleness/check.ts`:
- Around line 97-99: Thread the resolved frozen-mode state through the staleness
fetch flow so --no-frozen overrides auto-detection. Update fetchCurrentVersion
and fetchNpmVersion to accept the resolved state, and have callers pass
isFrozenMode({ frozen, noFrozen }) rather than calling isFrozenMode() without
options; use that explicit value for the early return that skips registry
access.
In `@packages/intent/tests/integration/source-policy-surfaces.test.ts`:
- Around line 13-15: Remove the duplicated changed properties from the type
annotation in the affected test declarations, keeping a single changed field
with the intended type; update all occurrences in the relevant test sections and
verify no duplicate changed keys remain.
In `@packages/intent/tests/skills-update.test.ts`:
- Around line 155-156: Restore the global fetch mock after each test: update the
setup around fetchSpy to use vi.stubGlobal, and extend the existing afterEach
cleanup to call vi.unstubAllGlobals() alongside vi.unstubAllEnvs().
---
Outside diff comments:
In `@packages/intent/src/shared/utils.ts`:
- Around line 252-269: Update scanOptionsFromGlobalFlags() to resolve and
include the effective frozen state in its returned options, preserving the
explicit --no-frozen value instead of leaving frozen undefined. Ensure
scanForIntents() passes this resolved options.frozen to
detectGlobalNodeModules() so caller flags take precedence over CI
auto-detection.
---
Nitpick comments:
In `@docs/security/lockfile.md`:
- Around line 62-65: Vary the sentence openings in the frozen-mode failure list
while preserving the existing meanings, exit codes, and commands; update the
bullets describing drift, unapproved skill-bearing sources, a missing
intent.lock, and malformed or unsupported intent.lock files.
🪄 Autofix (Beta)
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
Run ID: 905a73c0-a301-4fe7-becd-f0ef5c627125
📒 Files selected for processing (78)
.changeset/many-forks-cough.mdbenchmarks/intent/README.mdbenchmarks/intent/helpers.tsbenchmarks/intent/lockfile-scan.bench.tsdocs/cli/intent-list.mddocs/cli/intent-skills.mddocs/concepts/configuration.mddocs/config.jsondocs/security/lockfile.mdpackages/intent/README.mdpackages/intent/package.jsonpackages/intent/src/cli.tspackages/intent/src/commands/install/guidance.tspackages/intent/src/commands/list.tspackages/intent/src/commands/skills/approve.tspackages/intent/src/commands/skills/diff.tspackages/intent/src/commands/skills/generate-manifest.tspackages/intent/src/commands/skills/scan.tspackages/intent/src/commands/skills/stale.tspackages/intent/src/commands/skills/support.tspackages/intent/src/commands/skills/update.tspackages/intent/src/commands/support.tspackages/intent/src/core/excludes.tspackages/intent/src/core/git-adapter.tspackages/intent/src/core/intent-core.tspackages/intent/src/core/load-resolution.tspackages/intent/src/core/lockfile/baseline-drift.tspackages/intent/src/core/lockfile/hash.tspackages/intent/src/core/lockfile/lockfile-diff.tspackages/intent/src/core/lockfile/lockfile-state.tspackages/intent/src/core/lockfile/lockfile.tspackages/intent/src/core/manifest.tspackages/intent/src/core/secrets.tspackages/intent/src/core/skill-path.tspackages/intent/src/core/source-policy.tspackages/intent/src/core/types.tspackages/intent/src/discovery/register.tspackages/intent/src/discovery/scanner.tspackages/intent/src/discovery/walk.tspackages/intent/src/index.tspackages/intent/src/shared/cli-output.tspackages/intent/src/shared/env-flag.tspackages/intent/src/shared/mode.tspackages/intent/src/shared/types.tspackages/intent/src/shared/utils.tspackages/intent/src/skills/paths.tspackages/intent/src/skills/resolver.tspackages/intent/src/staleness/check.tspackages/intent/tests/baseline-drift.test.tspackages/intent/tests/cli.test.tspackages/intent/tests/core.test.tspackages/intent/tests/excludes.test.tspackages/intent/tests/git-adapter-failure.test.tspackages/intent/tests/git-adapter.test.tspackages/intent/tests/global-node-modules.test.tspackages/intent/tests/hash.test.tspackages/intent/tests/integration/pnp-berry-corepack.test.tspackages/intent/tests/integration/skills-frozen-cli.test.tspackages/intent/tests/integration/source-policy-surfaces.test.tspackages/intent/tests/lockfile-diff.test.tspackages/intent/tests/lockfile-state.test.tspackages/intent/tests/lockfile.test.tspackages/intent/tests/manifest.test.tspackages/intent/tests/mode.test.tspackages/intent/tests/public-lockfile-types.test.tspackages/intent/tests/resolver.test.tspackages/intent/tests/scanner.test.tspackages/intent/tests/secrets.test.tspackages/intent/tests/skills-approve.test.tspackages/intent/tests/skills-diff.test.tspackages/intent/tests/skills-generate-manifest.test.tspackages/intent/tests/skills-resolve-source-arg.test.tspackages/intent/tests/skills-scan.test.tspackages/intent/tests/skills-stale.test.tspackages/intent/tests/skills-update.test.tspackages/intent/tests/source-policy.test.tspackages/intent/tests/staleness.test.tstsconfig.json
…pdate test to stub global fetch
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/intent/src/cli.ts (1)
214-300: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the
skills staleandskills generate-manifestactions.The dispatcher now rejects both commands, making the package-maintainer workflows required by this PR inaccessible. Restore their handlers/options or revise the public contract before merge.
🤖 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 `@packages/intent/src/cli.ts` around lines 214 - 300, Extend the skills command dispatcher to recognize and route the previously supported `stale` and `generate-manifest` actions instead of falling through to the unknown-action failure. Restore each action’s corresponding handler and option wiring, including any command usage, examples, and imports needed to preserve the public package-maintainer workflows.packages/intent/src/core/intent-core.ts (1)
407-438: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate the lock before resolving from scanned metadata.
droppedNamesandresolveSkillUsecurrently consume potentially drifted metadata beforeassertApprovedLockState. A renamed or malformed skill can therefore produce resolution errors or suggestions instead of the required stale-lock rejection. Move the assertion immediately after scanning.Proposed fix
} = scanForPolicedIntents({ cwd, scanOptions: withFsCache(scanOptions, fsCache), coreOptions: options, context: projectContext, }) + + if (frozen || lockedResult.status === 'found') { + assertApprovedLockState(cwd, scanResult, hiddenSourceCount, lockedResult, { + rejectHiddenSources: frozen, + requireLockfile: frozen, + }) + } + if (droppedNames.includes(parsedUse.packageName)) { const lateRefusal = packageNotListedRefusal(use, parsedUse.packageName) throw new IntentCoreError(lateRefusal.code, lateRefusal.message) } @@ - if (frozen || lockedResult.status === 'found') { - assertApprovedLockState(cwd, scanResult, hiddenSourceCount, lockedResult, { - rejectHiddenSources: frozen, - requireLockfile: frozen, - }) - }🤖 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 `@packages/intent/src/core/intent-core.ts` around lines 407 - 438, Move the assertApprovedLockState call immediately after scanForPolicedIntents returns, before checking droppedNames or calling resolveSkillUse. Preserve its existing frozen and lockedResult.status conditions and options, while ensuring stale-lock validation occurs before any scanned metadata is consumed.
🧹 Nitpick comments (2)
packages/intent/src/core/lockfile/hash.ts (1)
432-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared validation/expansion preamble.
computeSourceContentHash(lines 437-453) andreadSourceContentForReview(lines 472-487) share an identical 4-block preamble:assertCanonicalPackageRelativePaths→realpathSync→collectSkillContentEntries→assertNoDuplicateKeys. Extracting this into a helper keeps both paths in sync and reduces maintenance risk.♻️ Proposed refactor
+function prepareContentEntries( + packageRoot: string, + entries: ReadonlyArray<SkillContentEntry>, + fs: ReadFs, +): { realPackageRoot: string; contentEntries: SkillContentEntry[] } { + assertCanonicalPackageRelativePaths( + entries.map((entry) => entry.relativePath), + 'skill path', + ) + const realPackageRoot = fs.realpathSync(packageRoot) + const contentEntries = collectSkillContentEntries( + fs, + packageRoot, + entries, + realPackageRoot, + ) + assertNoDuplicateKeys( + contentEntries.map((entry) => entry.relativePath), + 'skill content path', + ) + return { realPackageRoot, contentEntries } +} + export function computeSourceContentHash( packageRoot: string, entries: ReadonlyArray<SkillContentEntry>, fs: ReadFs = nodeReadFs, ): SourceContentHash { - assertCanonicalPackageRelativePaths( - entries.map((entry) => entry.relativePath), - 'skill path', - ) - - const realPackageRoot = fs.realpathSync(packageRoot) - const contentEntries = collectSkillContentEntries( - fs, - packageRoot, - entries, - realPackageRoot, - ) - assertNoDuplicateKeys( - contentEntries.map((entry) => entry.relativePath), - 'skill content path', - ) + const { realPackageRoot, contentEntries } = prepareContentEntries(packageRoot, entries, fs) const hashed = readHashEntries(contentEntries, fs, realPackageRoot) return { skills: entries.map((entry) => entry.relativePath).toSorted(compareStrings), contentHash: computeReviewedSourceContentHash( hashed.map((entry) => ({ relativePath: entry.key, content: entry.value, })), ), } } export function readSourceContentForReview( packageRoot: string, entries: ReadonlyArray<SkillContentEntry>, fs: ReadFs = nodeReadFs, ): Array<SourceContentReviewEntry> { - assertCanonicalPackageRelativePaths( - entries.map((entry) => entry.relativePath), - 'skill path', - ) - - const realPackageRoot = fs.realpathSync(packageRoot) - const contentEntries = collectSkillContentEntries( - fs, - packageRoot, - entries, - realPackageRoot, - ) - assertNoDuplicateKeys( - contentEntries.map((entry) => entry.relativePath), - 'skill content path', - ) + const { realPackageRoot, contentEntries } = prepareContentEntries(packageRoot, entries, fs) return readHashEntries(contentEntries, fs, realPackageRoot) .map((entry) => ({ relativePath: entry.key, content: entry.value, contentHash: `sha256-${createHash('sha256').update(entry.value).digest('hex')}`, isBinary: entry.isBinary, byteLength: entry.byteLength, })) .toSorted((a, b) => compareStrings(a.relativePath, b.relativePath)) }Also applies to: 467-498
🤖 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 `@packages/intent/src/core/lockfile/hash.ts` around lines 432 - 465, Extract the shared validation and expansion sequence from computeSourceContentHash and readSourceContentForReview into a helper that canonicalizes entry paths, resolves the package root, collects skill content entries, and rejects duplicate keys. Update both functions to call the helper and reuse its returned entries while preserving their existing hashing and review behavior.packages/intent/tests/content-review.test.ts (1)
29-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a test for the missing-source case.
assertSourceContentReviewsMatchthrows when!source(review identity not found in current sources), but this path is not tested. A simple test with a review whose identity doesn't match any current source would close the gap.🧪 Suggested test addition
it('rejects content changed between hashing and review', () => { expect(() => assertSourceContentReviewsMatch( [review('sha256-review')], [source('sha256-current')], ), ).toThrow('changed while it was being reviewed') }) + + it('rejects a review with no matching current source', () => { + expect(() => + assertSourceContentReviewsMatch( + [{ ...review('sha256-current'), id: 'bar' }], + [source('sha256-current')], + ), + ).toThrow('changed while it was being reviewed') + }) })🤖 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 `@packages/intent/tests/content-review.test.ts` around lines 29 - 47, Add a test in the assertSourceContentReviewsMatch suite covering a review identity that is absent from the current sources, using distinct review and source hashes. Assert that the call throws the expected missing-source error for the !source path, while preserving the existing matching and changed-content tests.
🤖 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 `@packages/intent/src/commands/skills/support.ts`:
- Around line 113-124: Update the details formatting in the hidden-source
mapping callback to sanitize or escape each untrusted source.name and provenance
path before including them in the output strings. Preserve the existing sorting,
provenance joining, and “provenance unknown” behavior while ensuring terminal
escapes, newlines, and bidi controls cannot appear raw.
---
Outside diff comments:
In `@packages/intent/src/cli.ts`:
- Around line 214-300: Extend the skills command dispatcher to recognize and
route the previously supported `stale` and `generate-manifest` actions instead
of falling through to the unknown-action failure. Restore each action’s
corresponding handler and option wiring, including any command usage, examples,
and imports needed to preserve the public package-maintainer workflows.
In `@packages/intent/src/core/intent-core.ts`:
- Around line 407-438: Move the assertApprovedLockState call immediately after
scanForPolicedIntents returns, before checking droppedNames or calling
resolveSkillUse. Preserve its existing frozen and lockedResult.status conditions
and options, while ensuring stale-lock validation occurs before any scanned
metadata is consumed.
---
Nitpick comments:
In `@packages/intent/src/core/lockfile/hash.ts`:
- Around line 432-465: Extract the shared validation and expansion sequence from
computeSourceContentHash and readSourceContentForReview into a helper that
canonicalizes entry paths, resolves the package root, collects skill content
entries, and rejects duplicate keys. Update both functions to call the helper
and reuse its returned entries while preserving their existing hashing and
review behavior.
In `@packages/intent/tests/content-review.test.ts`:
- Around line 29-47: Add a test in the assertSourceContentReviewsMatch suite
covering a review identity that is absent from the current sources, using
distinct review and source hashes. Assert that the call throws the expected
missing-source error for the !source path, while preserving the existing
matching and changed-content tests.
🪄 Autofix (Beta)
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
Run ID: 61829d78-f7ab-4c79-9b0c-cb9208945e3c
📒 Files selected for processing (31)
.changeset/many-forks-cough.mddocs/cli/intent-skills.mddocs/concepts/trust-model.mddocs/security/lockfile.mdpackages/intent/README.mdpackages/intent/src/cli.tspackages/intent/src/commands/list.tspackages/intent/src/commands/skills/approve.tspackages/intent/src/commands/skills/diff.tspackages/intent/src/commands/skills/support.tspackages/intent/src/commands/skills/update.tspackages/intent/src/core/intent-core.tspackages/intent/src/core/lockfile/content-review.tspackages/intent/src/core/lockfile/hash.tspackages/intent/src/core/manifest.tspackages/intent/src/core/skill-path.tspackages/intent/src/core/source-policy.tspackages/intent/src/core/types.tspackages/intent/src/index.tspackages/intent/tests/cli.test.tspackages/intent/tests/content-review.test.tspackages/intent/tests/core.test.tspackages/intent/tests/integration/skills-frozen-cli.test.tspackages/intent/tests/lockfile-state.test.tspackages/intent/tests/manifest.test.tspackages/intent/tests/public-lockfile-types.test.tspackages/intent/tests/skills-approve.test.tspackages/intent/tests/skills-diff.test.tspackages/intent/tests/skills-scan.test.tspackages/intent/tests/skills-update.test.tspackages/intent/tests/source-policy.test.ts
💤 Files with no reviewable changes (2)
- packages/intent/tests/public-lockfile-types.test.ts
- packages/intent/src/index.ts
✅ Files skipped from review due to trivial changes (6)
- packages/intent/src/commands/list.ts
- docs/concepts/trust-model.md
- packages/intent/README.md
- .changeset/many-forks-cough.md
- docs/cli/intent-skills.md
- docs/security/lockfile.md
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/intent/src/core/types.ts
- packages/intent/tests/skills-diff.test.ts
- packages/intent/src/commands/skills/diff.ts
- packages/intent/tests/cli.test.ts
- packages/intent/tests/source-policy.test.ts
- packages/intent/tests/lockfile-state.test.ts
- packages/intent/src/core/source-policy.ts
- packages/intent/tests/skills-scan.test.ts
- packages/intent/src/commands/skills/approve.ts
Summary
Add consumer-managed
intent.lockfiles so projects can review and record the exact skill content they approve from explicitly trusted dependency sources.The consumer workflow is:
Consumer workflow
intent skills scan,diff,approve, andupdate.intent.lockfiles at the consumer project root.(kind, id), keepingworkspace:fooandnpm:fooas distinct trust identities and approvals.SKILL.mdplus supportedreferences/,assets/, andscripts/files with deterministic SHA-256 content hashes.--yesbeforeskills updateaccepts trust-bearing content, manifest, capability, secret, MCP, or skill-path changes.intent.lockexists, reject drift during ordinaryintent loadand agent catalog generation so interactive agents cannot silently consume content that differs from the approved state.--frozen,INTENT_FROZEN, or non-interactiveCIdetection.Optional package manifests
skills/intent.manifest.jsonagainst installed package identity, skill paths, and per-skill hashes.Documentation, exports, and performance
@tanstack/intent.Summary by CodeRabbit
New Features
intent skillscommands for scanning, reviewing, approving, and updating approved skill sources.intent.locktracking for approved skill content and metadata.Documentation