fix(copilot-local): implement instructions bundle + local agent JWT - #8
Open
asklar wants to merge 3 commits into
Open
fix(copilot-local): implement instructions bundle + local agent JWT#8asklar wants to merge 3 commits into
asklar wants to merge 3 commits into
Conversation
The adapter declared supportsInstructionsBundle: true but never read instructionsFilePath/instructionsRootPath, so Copilot ran with no AGENTS.md/ HEARTBEAT.md operating contract and never recorded an issue disposition (Paperclip 'Missing Disposition' / clear_next_step recovery failures). execute() now loads the entry file plus sibling *.md bundle files, prepends them to the prompt, and passes --add-dir <instructionsRootPath>. Adds a focused test suite for the loader. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The adapter declared supportsLocalAgentJwt: true but never consumed ctx.authToken, so the spawned Copilot agent had no credential for the Paperclip API and every call returned HTTP 401 — blocking issue checkout and the final disposition. Forward ctx.authToken as PAPERCLIP_API_KEY (unless already set via config.env), matching qwen-local/cline-local. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Code review found the MAX_INSTRUCTIONS_BYTES cap was applied with a character-based String.slice(), which overshoots the byte cap (~2-4x) for multibyte content and can split a surrogate pair. Read the file as bytes and truncate via StringDecoder so only complete characters within the cap are kept. Adds a multibyte byte-cap regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughImplements the previously-declared but missing Copilot adapter capabilities: ChangesInstructions bundle and auth token forwarding
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Paperclip
participant execute
participant loadCopilotInstructionsBundle
participant CopilotCLI
Paperclip->>execute: run(ctx.authToken, config)
execute->>execute: buildCopilotRuntimeConfig(authToken)
execute->>execute: set env.PAPERCLIP_API_KEY
execute->>loadCopilotInstructionsBundle: read entry + sibling *.md files
loadCopilotInstructionsBundle-->>execute: prompt, addDir, notes
execute->>CopilotCLI: spawn with prepended prompt, --add-dir
execute->>Paperclip: onMeta(instructionsBundle provenance)
Related issues: Suggested labels: bug, enhancement, copilot-local Suggested reviewers: superbiche 🐰 A rabbit hops through markdown trees, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/copilot-local/tests/instructions-bundle.test.ts (1)
19-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression for
ctx.authTokenforwarding and override precedence.This suite locks down the bundle loader well, but the other bug fixed in this PR—setting
PAPERCLIP_API_KEYfromctx.authTokenunlessconfig.envalready provides one—still has no coverage here. A smallbuildCopilotRuntimeConfig/execute()test would protect the exact 401 regression this change is meant to close.🤖 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/copilot-local/tests/instructions-bundle.test.ts` around lines 19 - 129, Add a regression test for `buildCopilotRuntimeConfig` or `execute()` that verifies `ctx.authToken` is forwarded into `PAPERCLIP_API_KEY` when `config.env` does not already define it. Also cover override precedence by asserting `config.env.PAPERCLIP_API_KEY` wins over `ctx.authToken`, so the auth token mapping and env override behavior stay locked down.
🤖 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/copilot-local/src/server/execute.ts`:
- Around line 143-168: The fallback directory selection in execute() is still
using rootDir even when fs.readdir(rootDir) fails, which can send an unreadable
path to --add-dir. Update the logic around rootDir, entryFile, and the directory
scan so that when the configured root cannot be read, the fallback addDir is the
readable directory that contains instructionsFilePath (or otherwise a valid
directory for relative references), while still keeping the entry file in the
bundle. Apply the same fix in the other duplicated call sites noted by the
review.
---
Nitpick comments:
In `@packages/copilot-local/tests/instructions-bundle.test.ts`:
- Around line 19-129: Add a regression test for `buildCopilotRuntimeConfig` or
`execute()` that verifies `ctx.authToken` is forwarded into `PAPERCLIP_API_KEY`
when `config.env` does not already define it. Also cover override precedence by
asserting `config.env.PAPERCLIP_API_KEY` wins over `ctx.authToken`, so the auth
token mapping and env override behavior stay locked down.
🪄 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 Plus
Run ID: 426e5e2d-4475-4635-be04-a3b6b814c6ed
📒 Files selected for processing (3)
.changeset/copilot-instructions-bundle.mdpackages/copilot-local/src/server/execute.tspackages/copilot-local/tests/instructions-bundle.test.ts
Comment on lines
+143
to
+168
| const rootDir = instructionsRootPath | ||
| ? path.resolve(instructionsRootPath) | ||
| : path.dirname(path.resolve(instructionsFilePath)); | ||
| const entryFile = instructionsFilePath ? path.resolve(instructionsFilePath) : ""; | ||
|
|
||
| // Ordered, de-duplicated list of bundle files: entry first, then remaining | ||
| // top-level `*.md` files in the root (sorted for determinism). | ||
| const files: string[] = []; | ||
| const seen = new Set<string>(); | ||
| const pushFile = (candidate: string) => { | ||
| const resolved = path.resolve(candidate); | ||
| if (seen.has(resolved)) return; | ||
| seen.add(resolved); | ||
| files.push(resolved); | ||
| }; | ||
| if (entryFile) pushFile(entryFile); | ||
| try { | ||
| const dirEntries = await fs.readdir(rootDir, { withFileTypes: true }); | ||
| for (const dirEntry of dirEntries | ||
| .filter((e) => e.isFile() && e.name.toLowerCase().endsWith(".md")) | ||
| .sort((a, b) => a.name.localeCompare(b.name))) { | ||
| pushFile(path.join(rootDir, dirEntry.name)); | ||
| } | ||
| } catch { | ||
| // Root unreadable (or only an entry file was supplied) — fall back to entry. | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fallback addDir to the readable entry directory when the configured root cannot be read.
If instructionsFilePath loads but fs.readdir(rootDir) fails, this still returns addDir = rootDir, and execute() forwards that bad path to --add-dir. That breaks the intended unreadable-root fallback: the prompt contains the entry file, but relative references now resolve against the wrong directory (or Copilot rejects the arg entirely).
Proposed fix
- const rootDir = instructionsRootPath
+ const rootDir = instructionsRootPath
? path.resolve(instructionsRootPath)
: path.dirname(path.resolve(instructionsFilePath));
const entryFile = instructionsFilePath ? path.resolve(instructionsFilePath) : "";
+ let addDir = rootDir;
@@
try {
const dirEntries = await fs.readdir(rootDir, { withFileTypes: true });
@@
} catch {
// Root unreadable (or only an entry file was supplied) — fall back to entry.
+ if (entryFile) addDir = path.dirname(entryFile);
}
@@
return {
prompt: preamble,
- addDir: rootDir,
+ addDir,
notes,
chars: preamble.length,
loadedFiles,
};Also applies to: 232-235, 422-425
🤖 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/copilot-local/src/server/execute.ts` around lines 143 - 168, The
fallback directory selection in execute() is still using rootDir even when
fs.readdir(rootDir) fails, which can send an unreadable path to --add-dir.
Update the logic around rootDir, entryFile, and the directory scan so that when
the configured root cannot be read, the fallback addDir is the readable
directory that contains instructionsFilePath (or otherwise a valid directory for
relative references), while still keeping the entry file in the bundle. Apply
the same fix in the other duplicated call sites noted by the review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7.
What changed
copilot-local:execute()now loads the managed instructions bundle — the entry file (instructionsFilePath) plus sibling*.mdfiles ininstructionsRootPath— prepends it to the run prompt, and exposes the root to the CLI via--add-dirso relative references resolve. (loadCopilotInstructionsBundle()inpackages/copilot-local/src/server/execute.ts.)copilot-local:execute()now forwards the per-runctx.authTokenasPAPERCLIP_API_KEY(unless already set viaconfig.env), matchingqwen-local/cline-local.StringDecoder(a char-based slice overshoots the cap and can split a surrogate pair for multibyte content).tests/instructions-bundle.test.ts, 6 tests) covering load order, HEARTBEAT/disposition content reaching the prompt, dedup, entry-only fallback, unreadable-files fallback, and the multibyte byte-cap.Why
The manifest declared
supportsInstructionsBundle: trueandsupportsLocalAgentJwt: true, but neither was implemented. As a result the Copilot agent ran with noAGENTS.md/HEARTBEAT.mdoperating contract (so it never recorded an issue disposition → Paperclipsuccessful_run_missing_state/clear_next_step"Missing Disposition" recovery failures) and no API credential (every Paperclip API call returned HTTP 401). See #7 for full root-cause and evidence.qwen-local/cline-localalready implement both; this bringscopilot-localin line.Verified live in a running Paperclip instance: the Chief-of-Staff agent went 30/30 successful runs, the first onboarding task ("Hire your first engineer") reached
done, with zero "Missing Disposition" recoveries and zero 401s.Verification
pnpm -r buildcleanpnpm -r typecheckcleanpnpm -r testgreen (154 tests in copilot-local)pnpm check:packagesgreenpnpm exec changeset) for any user-visible package changeSummary by CodeRabbit
New Features
Bug Fixes