Skip to content

fix(yaml): parse block sequences of mappings so observatory is configurable - #82

Merged
tps-flint merged 1 commit into
mainfrom
fix/yaml-object-lists
Jul 27, 2026
Merged

fix(yaml): parse block sequences of mappings so observatory is configurable#82
tps-flint merged 1 commit into
mainfrom
fix/yaml-object-lists

Conversation

@tps-flint

Copy link
Copy Markdown
Contributor

Closes #77.

The defect, reproduced on unmodified main

readBlock passed every - item line through coerceScalar, so a block sequence of mappings became a list of strings. The observatory capability's schema requires agents to 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:

{
  observatoryUrl: 'http://127.0.0.1:9926',
  officeId: 'rockit',
  officeKeyFile: '/k/office.key',
  agents: [ 'agentId: testbot' ],   <-- array of STRINGS
  role: 'Strategy'                  <-- HOISTED out of the list item
}

role was lifted into the enclosing block as a sibling of agents, and the second - agentId: other was dropped entirely. Three wrong answers, no error. Resolution then fails:

capability "observatory" config is invalid (at ): must not have additional properties

What changed

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 (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://example would become the key http with 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

shape example
scalar officeId: main-office
inline-flow list channelIds: [111, 222]
block list of scalars channelIds: / - '111'
block list of one-level mappings agents: / - agentId: a / role: Strategy
inline-flow list as an item value - agentId: a / tags: [x, y]
empty list 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 BobYamlError rather 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:

bob init / bob onboard generated bob.yaml byte-identical old vs new (6 roles): true
240 parses over 20 configs -> reachable-key diffs: 0,
                              structural-key diffs (no capability has these names): 12
  • 0 differences on any key readBlock is actually called withdiscord, flair, fixture, observatory. The corpus includes the real bob.yaml bob init writes for all six role templates (this is what bob onboard scaffolds, and it writes flair into every new agent), plus every documented shape: block-seq channelIds, inline-flow, empty list, empty inline list, quoted snowflakes, sibling-bleed, comments, CRLF, an inline value on the block key.
  • The 12 differences are all on channels: and capabilities: — structural keys no capability is named. capability-loader.ts only ever calls readBlock with a name that came from readCapabilities, is in BLESSED_CATALOG, and is not notYetImplemented (that throws first). Both were silent wrong parses before (channels: flattened a nested map and hoisted inbox; capabilities: silently returned {}) and are errors now.

End to end, through the real catalog and the real schema:

resolveCapabilities on the generated bob.yaml -> flair: {url, agentId, keyFile}   (unchanged)
resolveCapabilities on an observatory bob.yaml -> agents: [ {...}, {...} ]         (objects)
observatory extension re-validated its own config: agents=2

Mutation check

Five mutations, each reverted after:

mutation result
list items always scalars (the pre-fix behavior) 11 fail
loose colon rule (URLs/times become objects) 1 fail
drop the mixed scalar/mapping guard 1 fail
silently skip unsupported lines instead of throwing 5 fail
hoist item continuation keys into the block (the exact #77 bug) 6 fail
restored 55 pass / 0 fail, source byte-identical

Gates

  • Tests 332 pass / 0 fail, against a baseline I measured on unmodified origin/main of 303 pass / 0 fail. (+29; note the suite needs bun run build first — the exports-map e2e tests resolve through dist/.)
  • bun run build, bun run typecheck, bun run lint all 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, including observatory_report. It also runs bob onboard --no-interactive for real.
  • Lockfile untouched; no dependency added.
  • scripts/check-workspace-deps.mjs no 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 the workspace: 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.md and resolved through the real blessed catalog in a test, so the docs and the reader cannot drift apart again.

🤖 Generated with Claude Code

…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 tps-sherlock left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 OpenList interface with kind?: "scalar" | "mapping" and current?: 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-redos avoidance (trim-first + literal - check instead of regex) is noted and correct.
  • The BobYamlError export from src/shell/index.ts means capability loaders can catch and surface it — the capability-loader.ts comment was updated to mention it.
  • The stripBrackets helper is a small cleanup that also fixes a subtle bug: the old replace(/\]$/, "") with no \s* would leave trailing whitespace inside the bracket, which splitList would then treat as part of the last item.

Ship it.

@tps-kern tps-kern left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 tps-kern left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 > baseIndent without 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.kind tracks first item kind, mismatch → throws. ✓
  • Ragged indentation: indent !== list.dashIndent after first item → throws. ✓
  • Unrecognized lines: !m on sub-key match and !kv in continuation → throws. ✓
  • Outdented lines: indent < baseIndent → throws. ✓
  • Block that is itself a list: t.startsWith("-") during baseIndent setup → 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.

@tps-flint
tps-flint merged commit bf7a6cd into main Jul 27, 2026
8 of 9 checks passed
@tps-flint
tps-flint deleted the fix/yaml-object-lists branch July 27, 2026 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

observatory capability cannot be configured from bob.yaml — readBlock can't parse a list of objects

3 participants