Prepared detailed implementation plan for GitHub issue
Public
Prepared detailed implementation plan for GitHub issue
Agent
Pi
Model
GPT-5.2 Codex
Started
Jan 20, 12:33 AM
Messages162
Downloads0
Reviewed GitHub issue and project context.
UnderstandDrafted consolidated comment with design decisions.
EditExpanded comment with detailed implementation instructions.
Edit
Analyze GitHub issue(s): https://github.com/badlogic/pi-mono/issues/645 this is an issue by me and you. we already went through all of this. i think its finally time to implement this garbage. analyze the issue and the codebase, tell me if you have any open questions so we can finalize the details, then go to town
For each issue:
Read the issue in full, including all comments and linked issues/PRs.
For bugs:
For feature requests:
Do NOT implement unless explicitly asked. Analyze and propose only.
Summary of issue #645 and linked items
Current code behavior
Concise implementation approach Phase 1: PackageManager
Phase 2: Loader
Phase 3: Bootstrap flow
Phase 4: Runner initializer and AgentSession reload
Phase 5: Reload and watch
Phase 6: Package management CLI
Affected files and changes needed
Open questions to finalize
If these are confirmed, I can move to implementation.
Extend the pi manifest handling to include prompts and themes and maybe context file paths. Current PiManifest includes skills and themes but loader only uses extensions. Add prompts and context entries to the manifest shape.
i think this also needs some talking about
Answers and details
Project .pi/settings.json: { "extensions": ["./project-ext"], "skills": { "includeSkills": ["docker-*"] } }
With current deep merge behavior, arrays override, not merge. Effective: extensions: ["./project-ext"] skills.includeSkills: ["docker-*"]
If you want additive merge for extensions, we need custom handling so project extensions are appended to global instead of replacing. Same question applies to skills includeSkills and ignoredSkills. Do you want additive merge or override for each array.
If loader owns it: Option A. Keep current behavior
Option B. Loader returns explicit prompt overrides
Option C. Hybrid
Given your note, Option A or C is minimal. Option B is a more complete centralization but adds more decisions to loader.
Skills recursion Yes. Current loadSkills searches ~/.pi/agent/skills and /.pi/skills recursively already. It also loads Codex and Claude locations with their specific formats.
Git update I will implement: if source has tag or commit, skip update when already at that ref. If no version, update to latest.
Manifest handling discussion Today PiManifest in extensions/loader.ts only checks pi.extensions, but the interface already includes themes and skills. It does not include prompts or context files.
We need to decide what pi field means for non extensions. Proposed options:
Option 1. pi field means explicit paths only, no auto discovery inside the package
Option 2. pi field is an augmentation, not a replacement
Option 3. pi field only for extensions, other types always use auto discovery
Given your earlier issue text, it sounds like Option 1 is intended. If no pi field, fall back to default discovery for extensions, skills, prompts, themes. If pi field exists, use only those entries.
Also need to decide path formats:
Open questions
Once you confirm, I will proceed to implementation.
as for merging extensions arrays, yeah that makes sense, merge local and global, warn on conflicts (same extension different version via npm or git for example).
what about themes and prompt templates?
that can stay as is, packaged extensions won't ship SYSTEM.md or AGENTS.md or APPEND_SYSTEM.md. that said, wouldn't it make sense to construct the system in such a way that the loader is responsible for ingesting and providing all the dynamic parts? extensions, skills, themes, prompt templates, AGENTS.md, APPEND_SYSTEM.md, SYSTEM.md? the default loader would just implement the current behaviour of looking in the default locations. A custom loader can provide those things whichever way it likes, no? Then main uses the default loader and passes that to createAgentSession in the SDK?
So i think that's also part of what the default loader will do. resolve skills in known locations, resolve skills from extension packages, provide them all as a nice list of SKILL.md files to load to createAgentSession?
approved
manifest handling i don't think we should include context files in manifests, but we must include prompts.
pi field means explicit paths only (either folders to which parts of auto-disocvery applies, or concrete files), full auto-discovery (like we do in ~/.pi/agent) only for extension packages without a pi field. those must then follow the folder and file layouts expected in ~/.pi/agent
path formats:
Is this consistent? got a better proposal?
open questions:
let's keep discussing based on the above, possibly give feedback and a proposal of the full thing
Notes and proposal based on your latest guidance
Clarification needed
Settings merge and removal of skill filters
Extensions array merging and conflicts
Themes and prompt templates in settings
Loader ownership of all dynamic parts This fits well. I suggest we formalize a Loader result that includes:
Default loader behavior
Custom loader behavior
SDK changes
Manifest handling proposal
Path formats based on your list This is consistent, with one change for naming.
If you want prompts or themes to be recursive within package manifests, say so. Otherwise I will keep non recursive to match the folder rules.
Open questions to finalize
Once these are locked, I can proceed to implementation.
A yes, SKILL.md
I want to get rid of the source flags. if people want to use skills from another harness, they should symlink or use customDirectories. i'm ok with this breaking change. Merging of global and project customDirectories with dedup is fine too. I have a feeling we won't need enabled. it's not used anywhere either afaikt! i.e. SettingsManager.getSkillsEnabled(). so that means skills can become a simple array like extensions as well? which references local files SKILL.md or folders (recursively find SKILL.mds), in addition to discovery in ~/.pi/agent/skills
themes and prompt templates, yes makes sense, tho i feel we should harmonize this with extensions and skills above? I kind of feel there's a general theme here we can apply to all 4 things: extensions, skills, prompts themes?
we auto-discovery in ~/.pi/agent and /.pi/
Then in addition to that auto discovery, settings.json can have
skills: ["folder", "file"] prompts: ["folder", "file"] themes: ["folder", "file"] extensions: ["folder", "file", "npm", "git"]
and the same rules as for auto-disovery apply? any skill, theme or prompt declared in an extension package (either by folder layout or via pi field in packages.json)
if people want to disable skills or extensions loading, they can use --no-skills and --no-extensions, no need to make the config super complicated
I think the loader should have getters to get the currently loaded extension runtimes, skill objects, prompttemplate objects, theme objects, contextFiles, systemPromptSource, and appendPromptSources
Loader then also has a reload() method that reloads all the things it was configured to load, subsequent getXXX() calls return the new objects. does that make sense?
the sdk then does ... nothing much. it just takes a loader instance which it binds to the AgentSession is creates, so the agent session can reload things at any time
so the loader really loads everything, including context files, system.md, etc. does that make sense? tui can also get access to the loader to set things up
new proposal
Proposal aligned with your updated direction
Summary of behavior
Auto discovery rules
Paths listed in settings.json
Extension packages
Loader shape I propose a Loader interface like:
type LoaderConfig = { cwd: string; agentDir: string; settingsManager: SettingsManager; packageManager: PackageManager; filters?: { extensions?: { enabled?: boolean; include?: string[]; exclude?: string[] }; skills?: { enabled?: boolean }; prompts?: { enabled?: boolean }; themes?: { enabled?: boolean }; }; };
type LoadResult = { extensions: LoadExtensionsResult; skills: Skill[]; skillWarnings: SkillWarning[]; promptTemplates: PromptTemplate[]; themes: ThemeInfo[] | Theme[]; contextFiles: Array<{ path: string; content: string }>; systemPromptSource?: string; appendSystemPromptSource?: string; };
interface Loader { load(): Promise; getResult(): LoadResult | undefined; }
Notes
SDK changes
Questions to confirm
If this matches, I will proceed to implement exactly this shape.
that loader interface is fucking garbage.
i think what we need is something more like Resources interface
interface ResourceLoader { getExtensions(): ExtensionLoaderResult (prev LoadExtensionsResult) getSkills(): { skills: Skill[], warnings: SkillWarning[] } getPrompts(): { prompts: PromptTemplate[], warnings PromptTemplateWarning[] } getThemes(): { themes: Theme[], warnings: ThemeWarnings[] } getAgentsFiles(): { agentsFiles: { path: string, content: string }[] } getSystemPrompt(): string? getAppendSystemPrompt(): string[]? async reload(): ?? }
then we have a DefaultResourceLoader that implemnents our default semantics, people can subclass it, or wrap it?
and createAgentSession takes a ResourceLoader, and AgentSession exposes the ResourceLoader and has a reload() method that rewires everything, and possibly emits an event to tui or other parties when reloading has finished?
I want this to be as simple and clean as possible
Understood. Here is a cleaner proposal aligned with your ResourceLoader shape.
Proposed interfaces
export interface ResourceLoader { getExtensions(): LoadExtensionsResult; getSkills(): { skills: Skill[]; warnings: SkillWarning[] }; getPrompts(): { prompts: PromptTemplate[]; warnings: PromptTemplateWarning[] }; getThemes(): { themes: Theme[]; warnings: ThemeWarning[] }; getAgentsFiles(): { agentsFiles: Array<{ path: string; content: string }> }; getSystemPrompt(): string | undefined; getAppendSystemPrompt(): string[]; // one or more fragments reload(): Promise<void>;}Notes
DefaultResourceLoader behavior
SDK and AgentSession changes
Open questions
If this is good, I will proceed and adjust the analysis for warnings types.
errors: Array<{ path: string; error: string }>
i think this shape is what we want for warnings/errors in general? maybe we just have warningsAndErrors, and add a type field "warning" | "error", an optional path (in case the resource wasn't laoded from disk), and a text: string?
Proposed unified diagnostics shape
export interface ResourceDiagnostic { type: "warning" | "error"; message: string; path?: string;}ResourceLoader getters return { data, diagnostics } for each resource type, or we add a single getDiagnostics that aggregates them.
Example per resource:
getSkills(): { skills: Skill[]; diagnostics: ResourceDiagnostic[] }This replaces skillWarnings and any future prompt or theme warnings.
Confirm if you want per resource diagnostics or one aggregated list.
DefaultResourceLoader behavior
note that we also have --extension and other cli flags that may add additional resources, no?
Yes. DefaultResourceLoader should take CLI overrides too.
Proposal
For now only extensionSources and disable flags are used by CLI. This keeps it extensible without more CLI flags.
Confirm if you want CLI to also accept explicit prompt, theme, skill sources.
does shouldn't be overrides though, should they be? the string[] at least, no? those are in addition to the auto discovery shit?
Agree. CLI lists should be additive.
Updated behavior
Confirm if you want --skills and --themes and --prompts flags to exist, or only extensions for now.
no aggreation, each getter returns diagnostics just for that resource tpe.
read @packages/coding-agent/src/cli/args.ts to see if we forgot something?
CLI args we need to update or remove
Proposal update for ResourceLoader + CLI
If you want to repurpose --skills to accept explicit paths, confirm and I will adjust parseArgs and help accordingly.
so, we already have --extension, i think we should have --skill (takes a SKILL.md) --theme (takes a theme.json) --prompt-template (takes a prompt template .md).
then we have --no-skills, --no-extensions --no-theme and --no-prompt-templates, and if we also specify --skill, --theme, --prompt-template, --extension, those get added back.
is that consistent?
Yes, consistent.
I will add:
Disable flags:
Behavior:
If this is final, I will proceed to implementation.
not yet, we now still need to figure out how package manager factors into all of this.
in settings, or via --extension, what if a user specifics npm: or git:? what should happen?
We need to define how source specs are handled in two contexts: settings.json and CLI flags.
Proposed behavior
Open decisions A. Should CLI npm or git sources be auto installed into global locations, without touching settings.json.
B. Should CLI npm or git sources trigger a settings change, same as pi install.
I suggest A for flags, B for install command. That keeps flags ephemeral and install explicit.
Please confirm:
Also decide whether --skill, --prompt-template, --theme should accept npm or git or only filesystem paths. If we allow npm or git, the package must include pi field or follow auto discovery rules.
npm: and git: are only for extensions, which themselves then can contain extension entry points, skills, prompts, and themes
Understood.
Behavior update
This means --skill, --theme, --prompt-template only accept filesystem paths. Same for settings.json entries in skills, prompts, themes.
Anything else to finalize before implementation.
i think they should go into a /tmp folder just for that run?
the bigger question is: is package manager auto-invoked at any point? or is the user supposed to use the additional cli args to manually trigger pulling in an extension specified in settings.json?
Proposal
CLI npm or git
Auto invocation of PackageManager
Optional behavior
Confirm if auto install on startup is acceptable, or if you want a prompt or a flag to opt out.
well, installing via package manager may take a while, so whoever constructs the default loader may also want to show visual feedback in the tui ... so we need a kind of progress thing.
We can add progress callbacks to PackageManager and ResourceLoader.
Proposal
type PackageProgress = { source: string; phase: "resolve" | "install" | "update" | "done"; message?: string };Interactive mode can pass a handler that updates a loader or status line.
If you want, I can add a lightweight interface like:
interface ResourceLoaderProgress { phase: "extensions" | "skills" | "prompts" | "themes" | "context" | "system" | "package"; message: string;}Confirm which shape you want.
what does package manager do concretely? where does it put the things?
Concrete PackageManager behavior proposal
Global install locations
npm root -g to find global node_modules. If source is npm:@scope/pkg@1.2.3, install via npm install -g @scope/pkg@1.2.3, then resolve path via node_modules/@scope/pkg.~/.pi/agent/git/<host>/<path>/<ref?> or similar. Use ~/.pi/agent/git/github.com/user/repo/ and checkout ref. If no ref, track default branch and update via fetch + reset.Local install locations
Temp installs for CLI flags
Actions
Pinning
If this matches, I will proceed.
what does package manager return to the loader then? would it itself be a resource loader?
PackageManager is not a ResourceLoader. It is a dependency of DefaultResourceLoader.
PackageManager returns resolved paths for extension sources:
DefaultResourceLoader then applies extension discovery rules inside those directories, or uses pi field if present.
So the chain is: settings.json or CLI sources -> DefaultResourceLoader -> PackageManager.resolve -> resolved directories -> extension discovery -> ExtensionLoaderResult, skills, prompts, themes.
If you want PackageManager to expose a higher level helper like resolveExtensionSources to return arrays of paths, I can add that, but it still is not a ResourceLoader.
ok. what if the project local settings.json has npm in it? will that also be installed to the global npm modules?
With the current decision to use npm root -g, yes. Any npm: source in project settings would still install into the global npm modules directory.
If you want project settings to install into a project local cache instead, I can add a local mode. That would need a rule like: project settings use /.pi/npm/node_modules while global settings use npm root -g.
what did the original issue say about that?
Issue #645 specifies:
npm root -g.<cwd>/.pi/npm/node_modules/.So if we follow the issue, project settings would be local only when using the -l flag in install. Otherwise global.
ignore the -l flag and other shit we might add as an interface to the lci later. all that will do is modify the settings.json, either global or local. the rest will then be handled by the default "pipeline". no?
Yes. The CLI install or other management commands only mutate settings.json, either global or local. After that, the default loader pipeline handles resolve and install on startup or reload. With npm root -g, both global and project settings will install into the global node_modules path unless we introduce a separate local install location later.
soo, can we install to ./pi/npm/node_modules easily? in case we find a npm: extension in the project local settings.json?
Yes. Use npm install --prefix <cwd>/.pi/npm <pkg> and resolve path from <cwd>/.pi/npm/node_modules/<pkg>. We can detect origin of the source. If it comes from project settings, use local install dir. If it comes from global settings, use npm root -g.
Confirm if you want this split behavior now.
confirmed.
so back to when package-manager is triggered. read @packages/coding-agent/src/main.ts where in this flow would we do that? in the defaultresourceloader? which i guess we construct on startup to get extension cli flags?
Yes. The default ResourceLoader owns PackageManager and should be used in both passes.
Flow in main.ts:
This keeps the package manager inside the default loader and ensures it runs during the early extension flag discovery.
Confirm if you want step 5 to reuse the same loaded result or to reload after setting flags.
well, flags can't modify anything wrt to resources, can theyß
Extension flags do not affect resource loading. We can load once in the first pass, then only apply flag values to the runtime before creating the session. No second reload needed unless you want to re-read settings.json after parsing non extension flags.
ok, and a user of DefaultResourceLoader would not have to cnstruct their own package manager, correct?
Correct. DefaultResourceLoader should create and own its PackageManager internally. We can still accept an optional PackageManager for tests, but callers should not need to construct one.
alright. what about models.json? and keybindings.json? do we have more .json files that could live in ~/.pi/agent or /.pi? what do we have atm? i think resourceloader should handle all those things, no?
Current config and data files
Global (~/.pi/agent):
Project (/.pi):
Project context files outside .pi:
Recommendation ResourceLoader should only handle runtime resources that affect the session prompt and extension system:
It should not handle:
If you want ResourceLoader to handle settings.json too, that is a larger refactor. My suggestion is to keep settings in SettingsManager and let DefaultResourceLoader read from it.
no, that sounds good to me. alright, does it make sense to encode ALL our decisions above into a new comment on the issue 645 that describes everything in detail?
Yes. A single comment on #645 that consolidates the final design and decisions will prevent drift. If you want, I can post it as a structured checklist covering ResourceLoader API, DefaultResourceLoader behavior, package manager rules, settings layout, CLI flags, and reload flow.
do that please. i suggest you first write to a local file, then upload that as a comment via gh cli
Comment posted: https://github.com/badlogic/pi-mono/issues/645#issuecomment-3770700174
If you want any edits to the comment, say which parts to adjust.
could you implement this based on just that comment? i HIGHLY doubt that ... add all the detalis needed, so the comment is self-conatined, contains all shapes, all files you need to read and modify, etc.
Posted expanded comment: https://github.com/badlogic/pi-mono/issues/645#issuecomment-3770703260
If you want any adjustments, list them and I will update with a follow-up comment.
export interface ResourceLoader { getExtensions(): LoadExtensionsResult; getSkills(): { skills: Skill[]; warnings: SkillWarning[] }; getPrompts(): { prompts: PromptTemplate[]; warnings: PromptTemplateWarning[] }; getThemes(): { themes: Theme[]; warnings: ThemeWarning[] }; getAgentsFiles(): { agentsFiles: Array<{ path: string; content: string }> }; getSystemPrompt(): string | undefined; getAppendSystemPrompt(): string[]; // one or more fragments reload(): Promise<void>;}export interface ResourceDiagnostic { type: "warning" | "error"; message: string; path?: string;}getSkills(): { skills: Skill[]; diagnostics: ResourceDiagnostic[] }type PackageProgress = { source: string; phase: "resolve" | "install" | "update" | "done"; message?: string };interface ResourceLoaderProgress { phase: "extensions" | "skills" | "prompts" | "themes" | "context" | "system" | "package"; message: string;}