Add Copilot package-resolution hook to the VS Code plugin - #42
Conversation
551ca00 to
ee6b7c1
Compare
sverdlov93
left a comment
sverdlov93
left a comment
There was a problem hiding this comment.
🤖 Review comment by Cursor AI
Deep review of this draft assembly PR (depends on jfrog-agent-hooks#48). CI validate job is green. ${CLAUDE_PLUGIN_ROOT} wiring is correct for a Claude-format plugin.
Findings (inline below)
- 🟠 Validation only proves pending/
NOT READY, never routing - 🟠 CI does not verify vendor pin integrity
- 🟠 Pin is an unmerged agent-hooks commit (
0a6e194on open #48) - 🟡 Pin file without sibling
sync-modules.mjsregenerator - 🟡 Workflow path filter omits the pin file
- 🟡 README prerequisites incomplete vs upstream smoke (
chat.plugins.enabled) - 🟡 PR description stale (still cites pin
b680ef…/ version1.0.10; tree is0a6e194/1.0.11)
🔵 Vendored cursor-/claude-session-start.mjs are unused at runtime — intentional wholesale modules/ sync, same as sibling plugins.
Merge order: land agent-hooks#48 → re-pin to merged SHA/tag → strengthen validation → un-draft. Do not merge this while the pin is off master.
Known carry-over (same as Claude/Cursor, not unique here): timeout: 7 + cold sequential verify → BUG-02 fail-open class.
| { | ||
| "type": "command", | ||
| "command": "node \"${CLAUDE_PLUGIN_ROOT}/modules/vscode-session-start.mjs\" package-resolution", | ||
| "timeout": 7, |
There was a problem hiding this comment.
🤖 Review comment by Cursor AI
🟡 Known carry-over: timeout: 7 + cold sequential verify (BUG-02 class)
Same budget as Claude/Cursor plugins. Identity probe ~3s + per-repo verify ~5s sequential over up to 8 types can exceed 7s; harness kills the process before stdout → session starts with no policy (fail-open). README documents fail-open intentionally. Not unique to this PR, but shipping APR to a third harness inherits the same cold-start hole — worth tracking against agent-hooks#25 BUG-02 rather than treating smoke-test success as coverage.
There was a problem hiding this comment.
Agreed this is a real risk, but it's inherited from claude-plugin/cursor-plugin's existing timeout: 7 budget, not introduced here — same cold-start hole applies to all three harnesses. Tracking against agent-hooks#25 BUG-02 rather than fixing per-plugin in this PR. Leaving open for visibility.
There was a problem hiding this comment.
Traced through the actual mechanism here. The per-type verify loop in resolver.mjs is a plain sequential for/await, not parallelized, and the shipped default config governs all 8 package types. So it's the identity probe (up to 3s) plus up to 8 sequential 5s calls, and it scales with however many types are governed. Also worth noting, writeCacheFile only runs once, after the whole loop finishes, so if the process gets killed at the timeout nothing gets cached and nothing gets written to stdout. That means it can fail open silently on every session on a slow network, not just the first one, since it never successfully completes and caches. A Promise.all on the per-type loop would probably fix most of this.
|
🤖 Review comment by Cursor AI @omerzi — naming decision requested on the upstream PR (same topic): please choose keep vscode host / copilot harness dual naming vs copilot-only surface naming. Full write-up: https://github.jfrog.info/JFROG/jfrog-agent-hooks/pull/48 (latest comment to @OmerZ). Short version: Claude-in-VS Code is still |
|
🤖 Review comment by Cursor AI @omerzi — added the Claude/Cursor comparison on upstream: https://github.jfrog.info/JFROG/jfrog-agent-hooks/pull/48#issuecomment-829544 (and the follow-up just posted) Claude/Cursor keep agent ≈ adapter file ≈ harness id. This PR alone splits |
- Register the dedicated VS Code SessionStart adapter. - Validate hook wiring and document the Node 20 requirement.
Keep VS Code SessionStart validation scoped to package resolution after Agent Guard moved to skill-based delivery, and refresh vendored modules from the rebased upstream branch.
Refresh the vendored package hook so VS Code uses the same positive Copilot discriminator as the upstream implementation.
Re-pin plugin/modules to agent-hooks 847774b and bump the plugin version so the package-resolution hook ships with the latest upstream fixes.
Picks up the upstream fix for harness detection, which previously handed any SessionStart payload it did not recognize to Claude. A VS Code payload whose source was not exactly "new" was labelled foreign and the adapter dropped the routing policy without injecting it.
Validation only ever exercised the unconfigured advisory, so a break in the routing branch — the one carrying the Artifactory URLs this plugin exists to inject — would have passed CI. Adds a configured fixture with a stand-in jf, and stops the checks inheriting a kill switch or redirected JFrog home from whoever runs them. Restores .github/scripts/sync-modules.mjs from the sibling plugins so the vendor pin has a regenerator beside it instead of being documentation, and aligns the pin file with their repo spelling and dest_prefix.
4c0933b to
95dc527
Compare
Mirrors the upstream jfrog-agent-hooks rename so the harness id (copilot), log context ([copilot]), and adapter filename match — the same convention already used for claude-session-start.mjs and cursor-session-start.mjs. Bumps the plugin to 1.0.13 and re-pins the vendor source to the upstream commit that ships the rename.
omerzi
changed the title
|
Went with option 2 on the naming thread — the vendored adapter is now |
sankerr
left a comment
sankerr
left a comment
There was a problem hiding this comment.
🤖 Review comment by Cursor AI
Senior review of #42 (head b3fc5e8) for teammate handoff, including sync-job + docs parity vs cursor/claude.
Prior round
Routing validation, path filter, README prerequisites, sync-modules.mjs, and re-pin to merged master SHA are addressed. Still open: pin↔tree integrity in CI; timeout: 7 cold-start (BUG-02) as shared carry-over.
Sync Plugins (jfrog-agent-hooks)
vscode-plugin is in .github/plugins.json / sync-plugins.yml. This PR supplies the downstream assembly the job needs. Do not run Sync Plugins from tag jfrog-agent-hooks/v0.9.0 after merge — that tag lacks copilot-session-start.mjs and would wipe the adapter. Cut a hooks release that includes 17df9b6, then re-pin to the tag.
Docs parity
No docs/ user/admin guides, no preview notice + guide links (cursor/claude both have them). README still links to missing CONTRIBUTING/SECURITY/LICENSE (pre-existing).
Findings (inline)
- 🟠 Missing APR user/admin docs vs sibling plugins
- 🟠 Pin is post-
v0.9.0; sync of that tag would remove the Copilot adapter - 🟡 PR description stale (Agent Guard flow, old pin/version, overstated test plan)
- 🟡 Test plan claims pin integrity; CI does not verify it
|
|
||
| After authentication, open a workspace in VS Code. The JFrog skills load on demand, the JFrog Agent Guard becomes active, and any MCP servers approved for your project become available to your Copilot agent. You can manage everything through natural language — no terminal commands required. | ||
|
|
||
| ### Agent Package Resolution |
There was a problem hiding this comment.
🤖 Review comment by Cursor AI
🟠 Missing APR user/admin docs vs sibling plugins
Cursor and Claude ship docs/package-resolution-user-guide.md, docs/package-resolution-admin-guide.md, a preview notice, and README links to those guides. This PR only adds a short Usage blurb — no docs/ folder and no guide links.
Shipping Copilot APR without the same user/admin guides leaves setup and governance undocumented for this harness.
Fix: Port the two guides (add VS Code prerequisites: chat.plugins.enabled, chat.useHooks), then add the preview notice + guide links in README to match cursor/claude.
There was a problem hiding this comment.
Confirmed there's no docs folder anywhere in this repo. The README section for this feature is about 11 lines covering where to enable it and what fail-open means, nothing on the config schema itself or what the NOT READY advisory means when someone hits it.
There was a problem hiding this comment.
🤖 Review comment by Cursor AI
🟡 PR description is stale vs HEAD
Body still describes:
- Agent Guard SessionStart injector alongside package resolution (flow diagram includes Agent Guard hook)
- Vendored pin
b680ef…and version1.0.10
HEAD is package-resolution-only SessionStart (copilot-session-start.mjs), pin 17df9b6, version 1.0.13. Test plan still claims Agent Guard force enable/disable remains intact and that vendored modules exactly match the pin in CI.
Fix: Refresh overview, mermaid flow, pin/version, and test-plan checkboxes to match current tree.
There was a problem hiding this comment.
The Agent Guard checkbox is checked, but there's nothing left to verify: hooks.json only has one hook now, and the author's own commit eafdc9b says outright that VS Code SessionStart validation was scoped to package resolution after Agent Guard moved to skill-based delivery. So that box covers a code path that doesn't exist anymore. Same thing with the Dependency section, it still reads as an open blocker on agent-hooks#48 even though that merged a while ago and the pin already points at the merged SHA. Separately from the description itself, this PR is out of draft and open for merge right now while this thread and the pin-tag one are both still sitting unanswered.
| @@ -0,0 +1,8 @@ | |||
| { | |||
| "repo": "JFROG/jfrog-agent-hooks", | |||
| "pin": "17df9b61b5cf78f49bf36d68e1c007f5db71e072", | |||
There was a problem hiding this comment.
🤖 Review comment by Cursor AI
🟠 Pin is post-v0.9.0; Sync Plugins on that tag would remove the Copilot adapter
pin = 17df9b61… (merged master SHA that adds modules/copilot-session-start.mjs). Tag jfrog-agent-hooks/v0.9.0 does not contain that adapter.
If Sync Plugins runs from v0.9.0 after this merges, it will replace plugin/modules/ with a tree that lacks copilot-session-start.mjs while hooks.json still points at it → SessionStart fails open with no routing.
Sibling plugins pin release tags (e.g. jfrog-agent-hooks/v0.8.1).
Fix: Cut an agent-hooks release that includes 17df9b6, re-pin this file to that tag, then allow Sync Plugins. Do not sync from v0.9.0.
There was a problem hiding this comment.
Can confirm the parts of this checkable from this repo: the pin does match copilot-session-start.mjs's current content, and hooks.json does reference that exact file as the command to run, so its absence would break the hook exactly as described. Worth folding in here too, nothing in the validate script or the workflow actually diffs plugin/modules against this pin value. sync-modules.mjs reads it but only logs it, never compares. So a re-vendor from the wrong ref would pass CI as long as copilot-session-start.mjs still exists and the version numbers match, even if the resolver logic underneath had drifted.
| console.log("\nAll checks passed."); | ||
| } | ||
|
|
||
| main(); |
There was a problem hiding this comment.
🤖 Review comment by Cursor AI
🟡 Test plan claims pin integrity; CI does not verify it
PR test plan checks “Verify vendored modules exactly match the pinned upstream revision,” but this script never fetches/compares plugin/modules/ against sync-modules-vendor.json’s pin. Assembly + routing fixtures are covered; pin↔tree drift would still pass CI.
Fix: Add a pin integrity check (or uncheck that test-plan item and track as an explicit follow-up).
| "enabled": false, | ||
| "verifyRepos": true, | ||
| "cacheTtlDays": 7, | ||
| "defaultGlobalRepos": { |
There was a problem hiding this comment.
This default config governs all 8 package types with placeholder repo keys (npm-virtual, pypi-virtual, and so on). verifyRepos defaults to true, and a governed-but-unresolved type stays governed and blocks installs instead of falling through. So the documented setup step of just flipping packageResolution.enabled to true, with nothing else changed, will block every npm/pip/maven/gradle/go/docker/helm/nuget install until someone edits these to real repo keys. Might be worth shipping this empty by default so enabling with no other config is a no-op instead of a full block.
|
|
||
| function isEntryFresh(entry, agentsConfigMtimeMs, cacheTtlDays) { | ||
| if (!entry?.cached_at) return false; | ||
| if (cacheTtlDays === 0) return false; |
There was a problem hiding this comment.
cacheTtlDays: 0 means opposite things in two places. Here it forces the cache to always be treated as stale, but in eager-setup-receipt.mjs's receiptWithinTtl, ttlDays <= 0 means the receipt never expires. An admin setting 0 to mean "don't trust stale data" would get the resolver re-verifying every session while the setup receipt trusts old results forever. Probably want these to agree on what 0 means.
| for (const [type, repoKey] of Object.entries(ws.config.repositories)) { | ||
| if (!repoKey || !PACKAGE_TYPES.includes(type)) continue; | ||
| declared.push(type); | ||
| SESSION.byType[type] = { |
There was a problem hiding this comment.
This writes workspace-declared repos straight into SESSION.byType with no fetchRepoConfig/repoMatchesPackageType check, unlike the admin defaultGlobalRepos path a bit further up, which does that verification when verifyRepos is true. .jfrog/local/package-resolution.json lives inside the project tree, so a committed file (or one on an unreviewed branch) can point installs at a repo key that was never checked to exist or match the package type, and can add package types the admin never configured at all. Feels like this should go through the same verification the admin path gets.
|
|
||
| function effectiveServerId(hint) { | ||
| if (hint) return hint; | ||
| return identityOrNull()?.serverId ?? "default"; |
There was a problem hiding this comment.
This falls back to the literal string "default" whenever identityOrNull() can't resolve a serverId. Combined with isEntryFresh only checking TTL and agentsConfigMtimeMs (no server URL check), someone who switches their active jf server between sessions could get a cache hit against verification results from the previous server for up to cacheTtlDays. Haven't confirmed whether jf config export always echoes serverId, so this depends on that, but worth a look.
| writeAgentsConf(home, { | ||
| packageResolution: { | ||
| enabled: true, | ||
| verifyRepos: false, |
There was a problem hiding this comment.
This is the only test that exercises the routing/configured path, and it sets verifyRepos: false. The shipped default is verifyRepos: true, so the code that actually runs on every real install (fetchRepoConfig, the per-type loop, the 5s timeout, the cache-write logic) has no coverage here. Same file has nothing exercising eager-setup.mjs, eager-setup-receipt.mjs, or setup-conflict.mjs either (close to 1500 lines combined), and nothing for a 401/403 from jf, a network failure mid-resolve, or malformed jf config export output. Separately, index.mjs calls both orchestrateEagerSetup and renderInstruction, which only avoid resolving the same types twice because of the SESSION singleton cache in resolver.mjs. That's correct today, but nothing here would catch it if a future change broke that invariant, it would just quietly double the network calls inside the 7s hook budget.
| - "marketplace.json" | ||
| - "scripts/validate-package-resolution-hook.mjs" | ||
| - ".github/scripts/sync-modules-vendor.json" | ||
| - ".github/scripts/sync-modules.mjs" |
There was a problem hiding this comment.
The paths filter doesn't include this workflow file itself, so a PR that only edits this file wouldn't retrigger the job to confirm the change still works.
Give the Copilot hook enough bounded time for a cold repository verification and assert the hook budget in the plugin validator.
4 participants
Overview
Adds the Copilot SessionStart package-resolution hook to the VS Code plugin. It vendors the shared agent-hooks modules, wires the dedicated Copilot adapter, and validates the assembled plugin behavior.
Details
copilot-session-start.mjs.modules/bundle at the pinned upstream agent-hooks revision and records provenance for repeatable syncs.Flow
Test plan
node scripts/validate-package-resolution-hook.mjs.Dependency
This PR will consume the release produced by agent-hooks PR #84, which hardens safe defaults, concurrent verification, workspace scope, cache identity, and TTL semantics.