fix(yaml): parse block sequences of mappings so observatory is configurable - #82
Conversation
…urable Closes #77. readBlock passed every `- item` line through coerceScalar, so a list of mappings became a list of strings. The observatory capability's schema requires `agents` to be a non-empty array of objects, which made the only value the reader could produce the one value the schema rejects — a blessed capability with no reachable configuration path. The reproduction was worse than the report. Given observatory: agents: - agentId: a role: Strategy - agentId: b the reader returned `agents: ["agentId: a"]`, hoisted `role` into the enclosing block as a sibling of `agents`, and dropped the second item entirely. Three wrong answers, no error. readBlock now supports a block sequence of one-level mappings: `- name: value` opens an item, deeper-indented `name: value` lines continue it, and values coerce exactly as a block's own sub-keys do. Mapping items are recognized with YAML's own rule — a colon followed by whitespace or end of line — so `- http://example` stays the string it has always been rather than becoming the key `http`. Everything outside that grammar now throws BobYamlError instead of being guessed at: nested mappings, nested lists, flow mappings, an empty `-`, a list mixing scalars with mappings, ragged indentation, unrecognized lines. That is the actual lesson here. The mechanism was never broken — the capability loads and registers its tool. What shipped was a reader that answered a question it did not understand, plausibly enough that nothing downstream noticed. A parser for a deliberately small subset owes its caller an error at the edge of that subset, not a guess. The error carries the block name and the 1-based line number and never echoes a value, since bob.yaml values are file paths and ids that sit next to secrets and errors end up in logs. No behavior changes on any key readBlock is actually called with: 240 parses across 20 configs — including the bob.yaml `bob init` writes for all six role templates — are identical before and after for discord, flair, fixture and observatory. The twelve differences are all on `channels:` and `capabilities:`, structural keys no capability is named and the loader never asks for, and every one of them turns a silent wrong parse into an error. Also documents the capability config grammar in the README, which had no example of a capability block at all. That absence is part of how this shipped: nobody had cause to write the block that turned out to be unwritable. The example is resolved through the real catalog in a test so the docs and the reader cannot drift apart again.
tps-sherlock
left a comment
There was a problem hiding this comment.
✅ Approved — the grammar is enforced, the invariant holds
Scope boundary: enforced, not merely described
The supported grammar is: scalars, inline-flow lists, block lists of scalars, block lists of one-level mappings, inline-flow as an item value, empty list. Every shape beyond that throws BobYamlError. The test suite has 18 cases in the "unsupported shapes throw" describe block — nested mappings, nested lists, lists of lists, flow mappings, empty dash, mixed scalar/mapping items, ragged indentation, outdented lines, unrecognized lines, a block that is itself a list, a list item with no sub-key opening a list, and more. Each asserts toThrow(BobYamlError). The boundary is the right one for a hand-rolled subset covering all four capability schemas' actual needs, and it is enforced.
Declined YAML library: the reasoning holds
The grammar covers every shape all four capability schemas need. A parser dependency is not warranted by this bug in a package whose packaging story is one tarball with no bundler. The revisit trigger — config growing past one level of nesting — is flagged in the header comment. If that happens, swap in a real parser rather than widening this one. The builder argued for not using a library; I agree.
Failure mode: no value ever echoed
I traced every throw path in readBlock and setMappingValue. All 16 throw sites use key and lineNo but never echo a value. The detail strings are all structural descriptions ("nested mappings are not supported", "a list item appeared where no name: opened a list", etc.). The test explicitly verifies this: a block with token: super-secret-value followed by a nested mapping throws at line 5, and the message does NOT contain "super-secret-value". BobYamlError's constructor takes key, line, and detail — never a value. ✅
Colon rule: correctly prevents silent wrong parse
matchKey() uses /^([A-Za-z0-9_-]+)[ \t]*:([ \t].*)?$/ — the colon must be followed by whitespace or end-of-line. This is YAML's own rule, and it's load-bearing: without it, - http://a.example would parse as key http with value //a.example, silently turning a list of URLs into a list of objects. The test "keeps a scalar item that merely CONTAINS a colon a scalar" verifies this with http://a.example, 09:00, and 'k: v'. This is the same class of silent-wrong-parse the fix removes. ✅
The observatory is now configurable — end-to-end verified
The test "resolves an observatory block written the documented way" walks the real catalog entry, the real schema, and the real bob.yaml path. It then hands the resolved config to the real observatory config loader (loadObservatoryConfig) which re-validates it — the agents list survives both the reader and the schema validator. The README's documented example is extracted from README.md and resolved through the real blessed catalog in a test, so the docs cannot drift from the parser. ✅
Regression evidence is solid
240 parses over 20 configs, zero differences on any key readBlock is actually called with (discord, flair, fixture, observatory). The 12 differences that do exist are on structural keys no capability is named, which the loader never passes to readBlock — and both were silent wrong parses before and are errors now. bob init and bob onboard produce byte-identical bob.yaml across all six role templates. ✅
Mutation checks: 5/5 caught
List-items-always-scalars (11 fail), loose colon rule (1), dropped mixed-shape guard (1), silently-skip-unsupported (5), hoisting item continuation keys — the exact bug (6). Restored byte-identical, 55 pass / 0 fail. ✅
Minor observations (non-blocking)
- The
OpenListinterface withkind?: "scalar" | "mapping"andcurrent?: Record<string, unknown>is clean — the state machine is explicit about what's open and what kind of items it's accepting. - The CodeQL
js/polynomial-redosavoidance (trim-first + literal-check instead of regex) is noted and correct. - The
BobYamlErrorexport fromsrc/shell/index.tsmeans capability loaders can catch and surface it — thecapability-loader.tscomment was updated to mention it. - The
stripBracketshelper is a small cleanup that also fixes a subtle bug: the oldreplace(/\]$/, "")with no\s*would leave trailing whitespace inside the bracket, whichsplitListwould then treat as part of the last item.
Ship it.
tps-kern
left a comment
There was a problem hiding this comment.
Approved — block sequences of mappings now parse correctly, unsupported shapes throw instead of silently producing wrong values.
The defect was worse than filed. The old readBlock parsed block sequences as scalars only. A list of mappings produced three distinct wrong answers simultaneously: mappings became strings, continuation keys were hoisted into the enclosing block, and subsequent list items were dropped. No error was raised. The observatory capability was effectively unconfigurable despite its package loading and registering fine. Good that the builder traced the full failure mode rather than just fixing the reported symptom.
Supported subset boundary is correct and enforced. Supported: scalars, inline-flow lists, block lists of scalars, block lists of one-level mappings, inline-flow as an item value, empty list. Explicitly unsupported and tested: nested mappings, nested lists, lists of lists, flow mappings, empty dash, mixed scalar/mapping items, ragged indentation, unrecognized lines, outdented lines. 17 unsupported shapes in the test matrix, each asserting BobYamlError. The boundary is enforced in code, not merely described in comments — every unsupported shape hits an explicit throw before it can produce a wrong value.
Hand-rolled parser vs. YAML library: correct call. The grammar covers every shape all four capability schemas need. Adding a YAML parser dependency to a package whose packaging story is one tarball with no bundler is not warranted by this bug. The revisit trigger is stated: config growing past one level of nesting. If that happens, swap in a real parser — don't widen this one. Agreed.
matchKey colon rule is load-bearing. The strict colon-must-be-followed-by-whitespace-or-EOL rule prevents - http://example from being parsed as key http with value //example. That is the same silent-wrong-parse class this fix exists to remove. Tested explicitly with URLs, time strings, and quoted values containing colons.
No value echoed in error messages. Every BobYamlError throw path includes only: block key, 1-based line number, sub-key name, and the SUPPORTED_SHAPES hint. No user values appear in any error string. The test names the block and the 1-based line, and never echoes a value explicitly asserts that super-secret-value does not appear in the error message. I checked all 14 throw sites — none interpolate a value.
Regression evidence is strong. Both old and new parsers built from source and diffed: bob init and bob onboard produce byte-identical bob.yaml across all six role templates. 240 parses over 20 configs gave zero differences on any key readBlock is actually called with. The 12 differences that exist are on structural keys no capability reads, and both were silent wrong parses before (now errors). Mutation check: 5 mutations, all caught.
README example is tested against the real catalog. The docs cannot drift from the parser — the example YAML in README.md is extracted and resolved through the real lookupCapability in a test. Good guard against the gap that let #77 ship in the first place (no documented example of a capability config block).
Adjacent findings noted, not fixed. Dependency audit red on brace-expansion is pre-existing and inside the 7-day supply-chain window. Not a defect in this PR.
Ship it.
tps-kern
left a comment
There was a problem hiding this comment.
Approved — block sequence of mappings support for bob-yaml, fixing #77.
The defect was worse than filed. The old reader didn't just miss list-of-mappings — it produced three distinct wrong answers with no error: agents became string array, second keys hoisted into enclosing block, subsequent items dropped. A capability that could never be configured shipped to publish. This fix is the right scope: make the supported shapes actually work, and make everything else fail loudly.
Scope boundary — correct subset, and it is ENFORCED. I traced every unsupported shape to its enforcement point:
- Nested mappings:
indent > baseIndentwithout an open list → throws. ✓ - Nested lists / lists of lists:
after.startsWith("-")→ throws. ✓ - Flow mappings (sub-key, item value, whole item): three separate
startsWith("{")checks → throws. ✓ - Empty dash:
after === ""→ throws. ✓ - Mixed scalar/mapping items:
list.kindtracks first item kind, mismatch → throws. ✓ - Ragged indentation:
indent !== list.dashIndentafter first item → throws. ✓ - Unrecognized lines:
!mon sub-key match and!kvin continuation → throws. ✓ - Outdented lines:
indent < baseIndent→ throws. ✓ - Block that is itself a list:
t.startsWith("-")duringbaseIndentsetup → throws. ✓ - Anchors/aliases (
&/*): don't match any recognized pattern → fall to!m→ throws. ✓
17 unsupported-shape test cases, each verified to throw BobYamlError. The boundary is enforced, not merely described.
No YAML library — agreed. The grammar covers every shape the four capability schemas need (verified by 240-parse diff over 20 configs). A parser dependency in a package whose packaging story is one tarball with no bundler is not warranted by this bug. The revisit trigger (config growing past one level of nesting) is explicitly documented in the header comment. When that fires, swap in a real parser — but not before.
No value echoing on any throw path — confirmed. Every BobYamlError construction carries only: block key (capability name), line number, sub-key name, and static description strings. No rest, no after, no rawLine, no coerced value. The test explicitly verifies expect(e.message).not.toContain("super-secret-value"). I checked all 15 throw sites — none interpolate a value. This matters because bob.yaml values sit next to secrets and errors land in logs.
The colon-must-be-followed-by-whitespace rule is correct. matchKey uses /^([A-Za-z0-9_-]+)[ \t]*:([ \t].*)?$/ — colon must be followed by whitespace or EOL. This prevents - http://example from being parsed as key http with value //example. Same rule, same load-bearing reason as the rest of the fix: a silent wrong parse is the defect class this PR exists to eliminate. Tested with URLs, time strings, and quoted colon values.
Regression evidence is solid. 240 parses, zero differences on any key readBlock is actually called with. The 12 differences on structural keys are all wrong-silent → error-now, which is the right direction. 5/5 mutation checks caught, including the exact bug (hoisting continuation keys → 6 failures). Restored to byte-identical 55/0.
The README test is a smart guard. Extracting the documented YAML example and resolving it through the real catalog prevents docs/parser drift — exactly the kind of test whose absence let #77 ship. The observatory integration test (resolve → config → extension's own loader re-validates) closes the loop end-to-end.
CI: 8/9 green. Dependency Audit red on brace-expansion — confirmed identical on main at the same base commit, inside the 7-day supply-chain window, not a defect in this PR. Build (TypeScript strict), Unit Tests, Published Artifact (all four capabilities register including observatory_report), CodeQL, Semgrep, Socket — all pass.
Verdict: Ship it. The boundary is the right subset for the current schemas, it is enforced rather than described, errors never echo values, and the README + catalog test prevents the docs-from-parser drift that let this ship in the first place. This was the last known blocker before bob can be pointed at anyone — it's clear.
Closes #77.
The defect, reproduced on unmodified
mainreadBlockpassed every- itemline throughcoerceScalar, so a block sequence of mappings became a list of strings. The observatory capability's schema requiresagentsto be a non-empty array of objects — so the only value the reader could produce was the one value the schema rejects. A blessed capability with no reachable configuration path.The reproduction is worse than the issue reported. Against a built
main, given a block with two agents and a second key on the first one, the reader returns:rolewas lifted into the enclosing block as a sibling ofagents, and the second- agentId: otherwas dropped entirely. Three wrong answers, no error. Resolution then fails:What changed
readBlocknow supports a block sequence of one-level mappings:- name: valueopens an item, deeper-indentedname: valuelines continue it, and values coerce exactly as a block's own sub-keys do (booleans, integers, quotes force a string,name: [a, b]inline lists).Mapping items are recognized with YAML's own rule — a colon followed by whitespace or end of line. Without that,
- http://examplewould become the keyhttpwith value//example, which is the same class of silent wrong parse this PR exists to remove. URLs and times in a scalar list stay scalars.Supported inside a capability block
officeId: main-officechannelIds: [111, 222]channelIds:/- '111'agents:/- agentId: a/role: Strategy- agentId: a/tags: [x, y]channelIds:with no items →[]Deliberately not supported — and now loud about it
Nested mappings (at block level or inside a list item), nested lists, lists of lists, flow mappings
{a: b}, an empty-, a list mixing scalar and mapping items, ragged list indentation, unrecognized lines, anchors, aliases, multi-line scalars.Every one of these throws
BobYamlErrorrather than being guessed at. That is the actual lesson of #77. The mechanism was never broken — the capability package loads and registers its tool fine. What shipped was a reader that answered a question it did not understand, plausibly enough that nothing downstream noticed. A parser for a deliberately small subset owes its caller an error at the edge of that subset, not a guess.The error carries the block name and the 1-based line number, and never echoes a value — bob.yaml values are file paths and ids that sit next to secrets, and error strings end up in logs.
I deliberately did not reach for a real YAML library. This grammar covers every shape the four capability schemas need, the boundary is now enforced rather than described, and adding a parser dependency to a package whose whole packaging story is "one tarball, no bundler" is a surface-area decision, not a bug fix.
Regression evidence
The risk that matters is an existing config changing meaning. Both parsers were built from source and diffed over a corpus:
readBlockis actually called with —discord,flair,fixture,observatory. The corpus includes the realbob.yamlbob initwrites for all six role templates (this is whatbob onboardscaffolds, and it writesflairinto every new agent), plus every documented shape: block-seqchannelIds, inline-flow, empty list, empty inline list, quoted snowflakes, sibling-bleed, comments, CRLF, an inline value on the block key.channels:andcapabilities:— structural keys no capability is named.capability-loader.tsonly ever callsreadBlockwith a name that came fromreadCapabilities, is inBLESSED_CATALOG, and is notnotYetImplemented(that throws first). Both were silent wrong parses before (channels:flattened a nested map and hoistedinbox;capabilities:silently returned{}) and are errors now.End to end, through the real catalog and the real schema:
Mutation check
Five mutations, each reverted after:
Gates
origin/mainof 303 pass / 0 fail. (+29; note the suite needsbun run buildfirst — the exports-map e2e tests resolve throughdist/.)bun run build,bun run typecheck,bun run lintall clean.Published Artifact(scripts/verify-pack.mjs) run locally with an isolated HOME and npm cache: PASS — packs, installs into a clean dir, and proves all four capabilities register their tools, includingobservatory_report. It also runsbob onboard --no-interactivefor real.scripts/check-workspace-deps.mjsno longer exists — it was removed by the single-package refactor (refactor(packaging): publish one @tpsdev-ai/bob, not six packages #79), which is also what removed theworkspace:specs it guarded.Also
The README had no example of a capability config block at all. That absence is part of how this shipped — nobody had cause to write the block that turned out to be unwritable. Added a "Configuring a capability" section documenting the grammar and the coercion rules, with the note that secrets are always a path, never a value. The example is extracted from
README.mdand resolved through the real blessed catalog in a test, so the docs and the reader cannot drift apart again.🤖 Generated with Claude Code