didericis/bot-bottle
1
0
Fork 0
Code Issues 7 Pull Requests 15 Actions Packages Projects Releases Wiki Activity

Defer broken manifest parse errors to preflight #239

Merged
didericis merged 8 commits from lazy-manifest-parse-on-select into main 2026-06-22 23:59:01 -04:00
Conversation 13 Commits 8 Files Changed 49
+634 -452
didericis-claude commented 2026-06-19 22:02:42 -04:00
Collaborator
Copy Link
Copy Source

Closes #236.

Summary

  • Per-file bottle/agent parse errors are now collected in Manifest.broken_agents rather than raised immediately during Manifest.resolve()
  • all_agent_names property exposes both valid and broken agent names for the CLI selector, so broken agents still appear in the picker
  • require_agent() and bottle_for() re-raise the stored error when a broken agent is selected, surfacing the problem only at preflight (after the user has selected and named their agent)
  • resolve_bottles_partial() in manifest_extends.py provides an error-tolerant bottle resolver that collects per-bottle failures without aborting the whole load
  • Agents referencing a broken bottle are also deferred, with a message pointing at the bottle error

Merge rule(s)

Unit tests only; integration tests do not need updating.

Closes #236. ## Summary - Per-file bottle/agent parse errors are now collected in `Manifest.broken_agents` rather than raised immediately during `Manifest.resolve()` - `all_agent_names` property exposes both valid and broken agent names for the CLI selector, so broken agents still appear in the picker - `require_agent()` and `bottle_for()` re-raise the stored error when a broken agent is selected, surfacing the problem only at preflight (after the user has selected and named their agent) - `resolve_bottles_partial()` in `manifest_extends.py` provides an error-tolerant bottle resolver that collects per-bottle failures without aborting the whole load - Agents referencing a broken bottle are also deferred, with a message pointing at the bottle error ## Merge rule(s) Unit tests only; integration tests do not need updating.
didericis reviewed 2026-06-19 22:23:10 -04:00
bot_bottle/manifest.py Outdated
@@ -262,3 +269,3 @@
from .manifest_loader import load_agents_from_dir, load_bottles_from_dir
bottles = load_bottles_from_dir(bottles_dir)
bottles, broken_bottle_errors = load_bottles_from_dir(bottles_dir)
didericis commented 2026-06-19 22:23:10 -04:00
Owner
Copy Link
Copy Source

Don't load all the bottles at this point anymore: wait to load the full bottle manifest (andly only for the relevant bottle) and catch errors during preflight. This should just load bottle names.

Don't load all the bottles at this point anymore: wait to load the full bottle manifest (andly only for the relevant bottle) and catch errors during preflight. This should just load bottle names.
didericis reviewed 2026-06-19 22:23:49 -04:00
bot_bottle/manifest.py Outdated
@@ -269,0 +275,4 @@
agents, broken_agents = load_agents_from_dir(
agents_dir, bottle_names, source="$HOME",
broken_bottle_errors=broken_bottle_errors,
)
didericis commented 2026-06-19 22:23:49 -04:00
Owner
Copy Link
Copy Source

Also don't load the agents here anymore, just load agent names/defer loading the full agent until preflight

Also don't load the agents here anymore, just load agent names/defer loading the full agent until preflight
didericis marked this conversation as resolved
didericis requested changes 2026-06-19 22:27:48 -04:00
didericis left a comment
Owner
Copy Link
Copy Source

General direction here is wrong: refactor should be a bit larger, and should change it from the current behavior of ALL bottles and ALL agents being loaded on start to ONLY the requested agent and ONLY the requested bottle (included any they inherit from) to be loaded. There should be no need to swallow errors: the errors should only occur for the particular bottle and particular agent during preflight, and there should be no earlier loads.

General direction here is wrong: refactor should be a bit larger, and should change it from the current behavior of ALL bottles and ALL agents being loaded on start to ONLY the requested agent and ONLY the requested bottle (included any they inherit from) to be loaded. There should be no need to swallow errors: the errors should only occur for the particular bottle and particular agent during preflight, and there should be no earlier loads.
didericis force-pushed lazy-manifest-parse-on-select from 0f201430b6 to f837211bf8 2026-06-22 15:40:34 -04:00 Compare
didericis-claude commented 2026-06-22 15:40:42 -04:00
Author
Collaborator
Copy Link
Copy Source

The latest commit addresses this — the refactor is larger as requested:

  • Manifest.resolve / from_md_dirs now only scans filenames; no file content is read at that point
  • all_agent_names (used by the agent picker) returns names from a filename scan, not from parsed content
  • Full parsing is deferred to load_for_agent, called from _validate inside BottleBackend.prepare — i.e. exactly at preflight, for only the selected agent and its bottle chain
  • Error swallowing is gone; parse errors surface naturally as ManifestError from load_for_agent / _validate

Branch has been rebased onto main (conflict in _validate — main had removed _validate_git_entries in the meantime; resolved by keeping the load_for_agent path). All tests pass, pyright clean.

The latest commit addresses this — the refactor is larger as requested: - `Manifest.resolve` / `from_md_dirs` now only scans filenames; no file content is read at that point - `all_agent_names` (used by the agent picker) returns names from a filename scan, not from parsed content - Full parsing is deferred to `load_for_agent`, called from `_validate` inside `BottleBackend.prepare` — i.e. exactly at preflight, for only the selected agent and its bottle chain - Error swallowing is gone; parse errors surface naturally as `ManifestError` from `load_for_agent` / `_validate` Branch has been rebased onto main (conflict in `_validate` — main had removed `_validate_git_entries` in the meantime; resolved by keeping the `load_for_agent` path). All tests pass, pyright clean.
didericis force-pushed lazy-manifest-parse-on-select from f837211bf8 to 0609813ba0 2026-06-22 15:43:15 -04:00 Compare
didericis reviewed 2026-06-22 15:46:38 -04:00
bot_bottle/manifest.py Outdated
@@ -193,6 +193,10 @@ class ManifestBottle:
class Manifest:
bottles: Mapping[str, ManifestBottle]
agents: Mapping[str, ManifestAgent]
didericis commented 2026-06-22 15:46:38 -04:00
Owner
Copy Link
Copy Source

Why are ALL the bottles and ALL the agents needed for a manifest?

Why are ALL the bottles and ALL the agents needed for a manifest?
didericis-claude commented 2026-06-22 15:47:44 -04:00
Author
Collaborator
Copy Link
Copy Source

from_json_obj is the test/programmatic path: the caller passes in an in-memory dict that already contains the full manifest, so there is nothing to defer — the data is already there. Lazy loading only makes sense when reading from the filesystem (the resolve → from_md_dirs path), where you want to avoid touching files you will never need.

The production start path (Manifest.resolve) now goes through from_md_dirs, which returns an empty bottles={}, agents={} manifest with just the root dirs recorded. No file content is read until load_for_agent is called at preflight.

`from_json_obj` is the test/programmatic path: the caller passes in an in-memory dict that already contains the full manifest, so there is nothing to defer — the data is already there. Lazy loading only makes sense when reading from the filesystem (the `resolve` → `from_md_dirs` path), where you want to avoid touching files you will never need. The production start path (`Manifest.resolve`) now goes through `from_md_dirs`, which returns an empty `bottles={}, agents={}` manifest with just the root dirs recorded. No file content is read until `load_for_agent` is called at preflight.
didericis commented 2026-06-22 15:51:49 -04:00
Owner
Copy Link
Copy Source

@didericis-claude that doesn't answer the question. When I launch an agent, I would expect the agent to only need information about the bottle and the agent used for that specific launch, meaning there should only be one agent and one bottle.

@didericis-claude that doesn't answer the question. When I launch an agent, I would expect the agent to only need information about the bottle and the agent used for that specific launch, meaning there should only be one agent and one bottle.
didericis-claude commented 2026-06-22 15:54:30 -04:00
Author
Collaborator
Copy Link
Copy Source

You're right — load_for_agent was returning self in the eager path, leaving all agents and bottles on the manifest. Fixed: both paths now return a manifest with exactly one agent and its bottle. Latest commit also updates load_for_agent's docstring to reflect this invariant.

You're right — `load_for_agent` was returning `self` in the eager path, leaving all agents and bottles on the manifest. Fixed: both paths now return a manifest with exactly one agent and its bottle. Latest commit also updates `load_for_agent`'s docstring to reflect this invariant.
didericis commented 2026-06-22 16:02:00 -04:00
Owner
Copy Link
Copy Source

@didericis-claude still not quite getting it. Ideally I'd like the manifest to be given a SINGLE bottle and a SINGLE agent. So I want the manifest bottle to be bottle: ManifestBottle, and the manifest agent to be agent: ManifestAgent, not mappings.

I'd like to do this so that the manifest ONLY applies to the data that's about to be launched, and so that we don't need to return a "names only" manifest in the resolve function.

@didericis-claude still not quite getting it. Ideally I'd like the manifest to be given a SINGLE `bottle` and a SINGLE `agent`. So I want the manifest bottle to be `bottle: ManifestBottle`, and the manifest agent to be `agent: ManifestAgent`, not mappings. I'd like to do this so that the manifest ONLY applies to the data that's about to be launched, and so that we don't need to return a "names only" manifest in the `resolve` function.
didericis-claude commented 2026-06-22 16:02:55 -04:00
Author
Collaborator
Copy Link
Copy Source

Got it — that's a cleaner design. Manifest becomes a value type holding exactly one agent: ManifestAgent and bottle: ManifestBottle; resolve (or a replacement) becomes a pure name-scanner returning list[str]; a full Manifest is only constructed once the specific agent is known.

That's a moderately large change — BottleSpec, all the backend prepare code, and a fair number of tests touch manifest.agents[...] / manifest.bottles[...]. Happy to do it in this PR if you want it landed here, or as a follow-up if you'd rather keep this PR focused. What's your preference?

Got it — that's a cleaner design. `Manifest` becomes a value type holding exactly one `agent: ManifestAgent` and `bottle: ManifestBottle`; `resolve` (or a replacement) becomes a pure name-scanner returning `list[str]`; a full `Manifest` is only constructed once the specific agent is known. That's a moderately large change — `BottleSpec`, all the backend prepare code, and a fair number of tests touch `manifest.agents[...]` / `manifest.bottles[...]`. Happy to do it in this PR if you want it landed here, or as a follow-up if you'd rather keep this PR focused. What's your preference?
didericis commented 2026-06-22 16:04:07 -04:00
Owner
Copy Link
Copy Source

@didericis-claude do it in here

@didericis-claude do it in here
didericis reviewed 2026-06-22 22:07:36 -04:00
bot_bottle/backend/__init__.py Outdated
@@ -62,3 +62,3 @@
availability) live on the plan, not the spec."""
manifest: Manifest
manifest: ManifestIndex | Manifest
didericis commented 2026-06-22 22:07:36 -04:00
Owner
Copy Link
Copy Source

Can you justify why a bottle needs to reference a manifest index? I don't see a great reason to have that be here. Try to argue for it/win me over.

Can you justify why a bottle needs to reference a manifest index? I don't see a great reason to have that be here. Try to argue for it/win me over.
didericis-claude commented 2026-06-22 22:22:33 -04:00
Author
Collaborator
Copy Link
Copy Source

re: #2144 — cannot justify the union. Clean split landed in latest commit: BottleSpec.manifest: ManifestIndex (always, CLI-supplied intent) and BottlePlan.manifest: Manifest (always, loaded by _validate()). _validate() returns the loaded Manifest directly; prepare() passes it to _resolve_plan(), which stores it on the plan. No union, no property, no type: ignore.

re: #2144 — cannot justify the union. Clean split landed in latest commit: `BottleSpec.manifest: ManifestIndex` (always, CLI-supplied intent) and `BottlePlan.manifest: Manifest` (always, loaded by `_validate()`). `_validate()` returns the loaded `Manifest` directly; `prepare()` passes it to `_resolve_plan()`, which stores it on the plan. No union, no property, no `type: ignore`.
didericis force-pushed lazy-manifest-parse-on-select from cadaa5dc25 to 31236b95a1 2026-06-22 23:05:40 -04:00 Compare
didericis force-pushed lazy-manifest-parse-on-select from 31236b95a1 to c6d0642a94 2026-06-22 23:43:11 -04:00 Compare
didericis added 8 commits 2026-06-22 23:54:12 -04:00
feat: defer broken manifest parse errors to preflight 3375df3f52 
Broken bottle/agent files no longer block the agent selector or prevent
unrelated agents from loading. Per-file parse errors are collected in
`Manifest.broken_agents`; the CLI selector includes them via
`all_agent_names`, and the error surfaces only when the specific agent
is selected and launch is attempted (in `require_agent`/`bottle_for`).

Closes #236
fix: resolve pyright reportUnusedImport in manifest_extends 996a260a98 
Import ManifestError at module level from manifest_util (no circular
dep) and remove the redundant local imports from function bodies that
were shadowing it. ManifestBottle retains its local import pattern to
avoid the circular manifest ↔ manifest_extends dependency.
refactor: scan filenames at resolve, parse only selected agent at preflight 3ccd09ed0d 
Manifest.resolve() now returns an empty-dict manifest with only directory
paths recorded (home_md, cwd_md). No content is read from any .md file
until load_for_agent() is called for a specific agent at preflight.

- Manifest.from_md_dirs: scan-only, no frontmatter parsing
- Manifest.load_for_agent: parses the selected agent file and its bottle
  chain; works on eager (from_json_obj) manifests too by returning self
- Manifest.all_agent_names: scans filenames in lazy mode
- backend._validate: calls load_for_agent and propagates upgraded spec
- cli/info.py, cli/list.py, cli/start.py: use load_for_agent / all_agent_names
- manifest_extends.py: reverted to original (no partial-resolve helpers)
- manifest_loader.py: only scan_agent_names + load_bottle_chain_from_dir
- Tests updated to call load_for_agent before accessing agents/bottles;
  test_md_agent_repos_deferred renamed to test_md_agent_repos_fails_at_preflight
fix: load_for_agent always returns single-agent manifest 2596c18954 
Filter to exactly one agent and one bottle in both the lazy (md-dirs)
and eager (from_json_obj) paths so the returned manifest invariant
holds regardless of how the manifest was constructed.
docs: clarify load_for_agent invariant in docstring 468ab8c290
refactor(manifest): split Manifest into ManifestIndex + Manifest single-value type 294a6ed023 
Manifest now holds exactly one agent and one effective bottle (with
git_user overlay already applied). The old multi-agent/bottle
collection is renamed ManifestIndex. BottleSpec.manifest starts as
ManifestIndex from the CLI and becomes Manifest after _validate()
calls load_for_agent(); all provisioning code downstream reads
spec.manifest.agent / spec.manifest.bottle instead of indexing by name.
fix(types): add BottleSpec.loaded_manifest to satisfy pyright on union type 56ef71060a 
BottleSpec.manifest is ManifestIndex | Manifest (pre/post _validate()).
Downstream code always runs post-validate so it needs Manifest, but
pyright flagged every .agent/.bottle access. The new loaded_manifest
property asserts isinstance and returns Manifest, giving pyright a
narrowed type without scattering type: ignore everywhere.

Also remove unused Manifest imports from test files and annotate the
_index() helper in test_manifest_agent_git_user.
refactor(types): move loaded manifest from BottleSpec to BottlePlan
test / integration (pull_request) Successful in 21s
Details
test / unit (pull_request) Successful in 49s
Details
lint / lint (push) Successful in 2m15s
Details
test / unit (push) Successful in 56s
Details
test / integration (push) Successful in 27s
Details
Update Quality Badges / update-badges (push) Successful in 2m37s
Details
da42740156 
BottleSpec.manifest was ManifestIndex | Manifest — a union encoding
two lifecycle stages in one field. The union was unjustifiable:
it forced a type-narrowing workaround (loaded_manifest property)
on every consumer.

Clean split:
- BottleSpec.manifest: ManifestIndex (always; CLI-supplied intent)
- BottlePlan.manifest: Manifest (always; loaded by _validate())

_validate() returns the loaded Manifest directly. prepare() passes
it to _resolve_plan(), which stores it on the plan. All provisioner
code now reads plan.manifest.agent / plan.manifest.bottle — no
union, no asserts, no type: ignore.
didericis force-pushed lazy-manifest-parse-on-select from c6d0642a94 to da42740156 2026-06-22 23:54:12 -04:00 Compare
didericis merged commit da42740156 into main 2026-06-22 23:59:01 -04:00
didericis deleted branch lazy-manifest-parse-on-select 2026-06-22 23:59:02 -04:00
Sign in to join this conversation.
Reviewers
No Reviewers
didericis
Labels
Clear labels
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Low
4
The priority is low
Priority
Medium
3
The priority is medium
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
didericis-claude (didericis (claude)) didericis-codex (didericis (codex)) didericis-gemma (gemma) didericis
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: didericis/bot-bottle#239
Delete Branch "lazy-manifest-parse-on-select"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?