Repository navigation
fix(core): read Claude instructions after plugin context hooks - #61273
pjiang000-eng wants to merge 3 commits into
Conversation
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix works. I ran the issue's reproduction through the claude-agent plugin harness for context, generate and compaction. On the base, system text pushed by a later hook is missing from claudeCodeInstructions (""). With this change it is included for all three. On the real request path the getter is safe: every hook gets the same event object, and SessionModelRequest.prepare only reads shaped.options after all hooks have run. The claude-agent tests pass (82/82).
Public API: the plugin API keeps its shape. One plugin-visible change: claudeCodeInstructions on event.options is now a getter with no setter (see the inline comment).
| event.options[ClaudeSystemPrompt.INSTRUCTIONS_OPTION] = instructions(event.system) | ||
| // Built-in providers register before user plugins, so `event.system` still grows after this hook | ||
| // returns. Read it when the request is built instead of folding it into a string here. | ||
| Object.defineProperty(event.options, ClaudeSystemPrompt.INSTRUCTIONS_OPTION, { |
There was a problem hiding this comment.
This property has a getter but no setter. Before, a later plugin hook could set event.options.claudeCodeInstructions = "..." to override what Claude receives. Now that assignment throws TypeError: Attempted to assign to readonly property in strict-mode code (ES modules, so plugins), which fails the hook. Adding a setter would keep overrides working, for example one that redefines the property as a plain value: set: (value) => Object.defineProperty(event.options, key, { value, enumerable: true, configurable: true, writable: true }).
There was a problem hiding this comment.
Good catch — added the setter, so a later hook assigning event.options.claudeCodeInstructions = "..." still wins; it redefines the key as a plain writable value. Covered by a new case in ClaudeAgentPlugin session hooks: without the setter that assignment throws TypeError: Attempted to assign to readonly property under ESM. claude-agent tests 83/83, bun run check clean.
There was a problem hiding this comment.
The earlier point about the missing setter is fixed: a later plugin can assign claudeCodeInstructions again, and the new test covers it. The claude-agent tests pass (86/86), along with the issue's reproduction for context, generate and compaction. Nothing new to raise. Public API unchanged.
|
Thanks for this. Two things I noticed while working on the same bug:
Tagging the base prompt and identity parts in their metadata and skipping by tag fixed both for me. |
|
Both are right, and I checked:
Tagging the base prompt and identity parts and skipping by tag is the right shape. Are you planning to open a PR for it? If not I'd rather fold it into this one than have two competing diffs on the same function — happy to take it either way. |
|
Are you planning to open a PR for it? no, cause you are already working on it, I built a fix on top of yours, here it is if you wanna take a look at it: Project579@97ddcb9dcd |
Issue for this PR
Closes #61248
Type of change
What does this PR do?
The provider joins
event.systemintoclaudeCodeInstructionsinside its owncontexthook. Built-in providers register before user plugins and hooks run in registration order, so a plugin that pushes a system part later never reaches it. The option is now an enumerable accessor overevent.system, read when the request is built so later hooks are included, with a setter that collapses it back to a plain value so a plugin can still assign an override. The value stays a string; no consumer changes.How did you verify your code works?
The new case in
ClaudeAgentPlugin session hooksfails ondev(Received: "") and passes here. The repro from the issue passes. claude-agent tests 83 pass / 0 fail,bun run checkclean. A second case pins the override path: without the setter, assigning the option throwsTypeError: Attempted to assign to readonly propertyunder ESM.Visual
Screenshots / recordings
Not a UI change.
Checklist