Commit c69f7eb
authored
feat: read-only YAML fragment lookup for configuration and package keys (#1882)
* fix: honour the configured packages folder when listing files
read_file resolves the packages folder bound via `homeassistant: packages:`
and allows reading `*.yaml` under it (#1854), but list_files was never given
the same treatment: it matched `rel_path` against ALLOWED_READ_DIRS alone, so
a packages folder was readable file-by-file yet could not be enumerated.
Pass the detected package dirs through to the allow decision, mirroring the
read path. The widening happens inside `_is_path_allowed_for_dir`, after the
deny floor and containment checks, so `.storage` and an escaping symlink stay
blocked.
`package_dirs` defaults to None and only the lister passes it: write_file and
delete_file share this helper and must not gain package access, since
edit_yaml_config is the only write path that may reach config YAML. The
folder is matched literally, as `_path_in_package_dir` does, so a folder name
containing glob metacharacters is not expanded.
* feat: return a parsed view alongside read_file's yaml_path subtree
`read_file`'s `yaml_path` returns the round-trip text of a subtree; an agent
inspecting config also wants it as data. Add `include_parsed` (default off,
so the existing auto-backup caller is unaffected), which returns the same
subtree as JSON-safe plain Python from a single parse.
HA tags are rendered to their SOURCE form (`!secret api_key`), never
resolved. The value behind a `!secret` lives in secrets.yaml and is not read
here, so the parsed view carries no plaintext-secret surface — the same
property the text view already had. `yaml_jsonify` lives in yaml_rt.py
because that module owns the tag registry.
services.yaml gained `include_parsed` and, while here, `yaml_path`: the
latter has been in the service schema since #1579 but was never declared,
so it was invisible to the UI service picker.
* feat: add ha_config_get_yaml for read-only YAML fragment lookup
`ha_config_set_yaml` can edit configuration.yaml, packages/*.yaml and
themes/*.yaml, but there was no matching read path: an agent had to fetch a
whole file through ha_read_file and parse it, and could not find which file
defines a key. Closes #1788.
`ha_config_get_yaml(yaml_path, file)` returns the fragment under a key, keyed
by file. `file` accepts an fnmatch glob, so `packages/*.yaml` searches a
directory in one call and reports only the files that define the key —
`include_content=False` reduces that to pure discovery. The returned `file` +
`yaml_path` are exactly the arguments that address the same fragment for an
edit, and `content` round-trips back unchanged.
It lives in its own module so it can register unconditionally: reading a
fragment is not an edit, and ENABLE_YAML_CONFIG_EDITING gates editing.
`ha_read_file` also forwards `yaml_path` now, for the single-file case within
the filesystem toolset.
Two deviations from the shape proposed in the issue, both verified against
the code rather than assumed:
- No `config_hash`. `ha_config_set_yaml` takes no such argument — it locks
with `confirm_token`, derived from the path plus the NEW content being
written, which a read cannot produce.
- `include_parsed` defaults off rather than on. `content` is the
round-trippable view; returning both by default doubles the payload for one
fragment.
MIN_COMPONENT_VERSION moves to the pending 1.1.0: the glob needs the lister's
packages-folder fix, and `include_parsed` is an argument an older strict
schema rejects with a raw voluptuous error. Both would otherwise surface as
confusing failures rather than an actionable update prompt. COMPONENT_VERSION
itself is left alone — master already leads stable 1.0.4, so this rides the
open pending version.
* test: pin the root-level glob boundary for ha_config_get_yaml
A `file` glob with no directory part resolves to the config root, which the
lister denies — root files stay readable one-by-one via an explicit `file`.
Pin the "." the tool sends so the deny is a deliberate boundary rather than a
malformed path.
* perf: fan out ha_config_get_yaml's per-file reads concurrently
A glob resolved to N files and then read them one at a time, paying N
sequential service round-trips to HA for what are independent reads — a
packages glob is routinely 10+ files. Gather them instead, matching the
fan-out pattern already used for per-item service calls elsewhere in
src/ha_mcp/tools. gather preserves order, so matches stay sorted by file and
the response shape is unchanged.
`_unwrap_or_raise` loses its `async`: it never awaited anything.
* fix: drop a provably unreachable return in ha_config_get_yaml
CodeQL flagged py/unreachable-statement. The trailing `return None` was
copied from the sibling tools, where it guards a different shape: their try
block ends in a `raise_tool_error(...)` call that CodeQL cannot see through,
so the fall-through looks reachable there. This try block ends in a real
return, and `exception_to_structured_error` is typed NoReturn, so every path
returns or raises and the trailer is dead code.
* perf: cache packages-folder detection behind an mtime signature
Every file operation resolves the configured packages folder, which parsed
configuration.yaml (and whatever it includes) from disk each time. A
ha_config_get_yaml glob makes that N+1 parses per search, concurrently.
Cache the detection per config path, keyed on the mtimes of every file the
loader actually read. The loader now records those paths, so an edit
invalidates regardless of whether the packages directive sits in
configuration.yaml or in an !include it follows — keying on the root file
alone would serve a stale allowlist when the homeassistant: section is split
out. A not-yet-existing include target is stamped -1, so creating it later
invalidates too.
Stat-per-file is cheap; the parse is not. Concurrent first-callers may each
miss once before the entry lands, which is bounded and self-healing, and
cheaper than holding a lock across executor threads.
Reported by Gemini on #1882.
* fix: restore the explicit None return inside ha_config_get_yaml's handler
The previous commit removed both trailing returns and traded
py/unreachable-statement for py/mixed-returns at the function head: CodeQL
does not see that exception_to_structured_error is typed NoReturn, so with no
explicit return the handler reads as falling through to an implicit None.
Keep the explicit return inside the except block, and keep omitting the one
after the try/except. The sibling tools carry both because their try block
ends in a raise_tool_error() call CodeQL treats as returning normally, which
makes their post-try trailer reachable; this try ends in a real return, so
that trailer is provably dead here.
* fix: do not read a broken or tailed file as a key that is absent
Two ways the fragment read could report "the key is not there" when it had in
fact never looked, both reported by Codex on #1882.
A file whose YAML does not parse returned subtree=None, which the tool treated
exactly like a file that parses but lacks the key. One broken package in a
`packages/*.yaml` glob therefore read as a clean "not defined anywhere".
_extract_yaml_views now also sets parse_error, and ha_config_get_yaml surfaces
those files as warnings instead of silently skipping them. The error carries
the position but never ruamel's message: that message embeds the offending
source line, which would put file content, possibly an inline credential, into
a response this path otherwise keeps free of resolved values.
read_file applied tail_lines to the content BEFORE extracting yaml_path from
it, so the key was looked up in the retained tail rather than the file: for a
key outside the tail, or a tail that is not valid YAML alone, the subtree came
back null although the key exists. Extraction now runs on the untailed text;
tailing stays a display concern. The ordering predates this PR (#1579), but
only the auto-backup used yaml_path and it never passes tail_lines, so
surfacing yaml_path on ha_read_file is what makes the combination reachable.
Covered by a component test that a parse error is reported without echoing
file content, a tool test that a broken file warns rather than counting as a
non-match, and an e2e that tail_lines plus yaml_path still resolves the key.
* fix: gate ha_config_get_yaml behind filesystem tools; warn per file under a glob
The tool returns config-file contents through the same read_file/list_files
component services as ha_read_file/ha_list_files, which are gated behind
enable_filesystem_tools. Registering it unconditionally handed an install that
turned filesystem tools off a config-read surface anyway. It now registers
behind that flag, carries the beta tag like its siblings, and has a
FEATURE_GATED_TOOLS stub so it stays discoverable in the settings UI when the
flag is off. The YAML *editing* flag still does not gate it: reading a fragment
is not an edit.
Under a glob, a file that cannot be searched no longer sinks the whole search
and discards the matches already found. A success=False payload, a read that
raises, and a malformed non-dict response all degrade to a warnings entry, the
way a parse error already did. The glob is not restricted to *.yaml, so
packages/* turning up a README made this reachable without contrivance. A
single explicit file target and a list_files failure still raise, since neither
has anything to salvage.
yaml_jsonify: a bool carrying an anchor loads as ruamel's ScalarBoolean, which
subclasses int but not bool, so it serialized as 1/0. It now has its own branch
ahead of int. Non-finite floats render to their YAML source form rather than
producing output that strict JSON rejects.
Tests: both gating directions, the three glob degrade paths, the raise paths,
an empty glob, a present-but-null key, the anchored-bool/non-finite/timestamp
branches, secrets.yaml masking under the yaml_path views (source guard plus an
e2e behavioural half), and the get to set round-trip that is the point of the
feature.
* fix: quote the secrets.yaml mask marker so it re-parses as a scalar
_mask_secrets_content emitted 'key: [MASKED]'. That text is itself valid YAML
and is now re-parsed: read_file's yaml_path/include_parsed views load it, and
an unquoted [MASKED] is flow-sequence syntax, so the parsed view rendered the
mask as the list ['MASKED'] rather than a scalar. Not a leak - the value stays
masked either way - but the structured view misrepresented a security-relevant
file.
The marker is now quoted. Every pre-existing consumer asserts the '[MASKED]'
substring against the text, which a quoted marker still satisfies; the five
tests that pinned the exact line format are updated, and a new test pins the
re-parse itself.
Nothing re-parsed the masked text before this PR, so the wart was unreachable.
The include_parsed view added here is its first consumer, and the e2e masking
test the review asked for is what surfaced it.
* test: cover the packages-glob skip and multi-match paths against the real component
The glob's warn-and-continue path was pinned only at the unit layer, where
read_file's refusal is a mock. The interaction it exists for -- the component's
.yaml-only package read rule rejecting a file that `custom_packages/*`
legitimately matched -- happens only against the real component, so it needs the
behavioural half too.
Nothing can create a non-YAML file inside a packages folder at test time:
write_file is never granted package access, and a post-boot host write to the
bind-mounted config dir doesn't propagate in CI. So the file is staged pre-boot
in the fresh-config fixture, next to the legacy backups, and the test skips on
the HAOS backends, which boot a pre-baked image carrying no such seed. HA loads
an !include_dir_named folder through _find_files(loc, "*.yaml"), so the seeded
.md is inert at boot.
Adds the count > 1 case as well: the same key defined in two package files is
the multi-match result shape the issue centers on, and no e2e produced it.
* fix: annotate ha_config_get_yaml with openWorldHint
The annotation became mandatory on master after this branch opened, and its
default is true, so the tool would ship silently marked open-world. False: the
tool's domain is the local Home Assistant instance, and it hands back the
operator's own config text -- the same call the other file-read tools make.1 parent 1ef44a5 commit c69f7eb
12 files changed
Lines changed: 1954 additions & 49 deletions
File tree
- custom_components/ha_mcp_tools
- src/ha_mcp
- settings_ui
- tools
- tests/src
- e2e
- workflows/filesystem
- unit
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
96 | 96 | | |
97 | 97 | | |
98 | 98 | | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
99 | 119 | | |
100 | 120 | | |
101 | 121 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| 5 | + | |
5 | 6 | | |
6 | 7 | | |
7 | 8 | | |
| 9 | + | |
8 | 10 | | |
9 | 11 | | |
10 | 12 | | |
11 | 13 | | |
| 14 | + | |
12 | 15 | | |
13 | 16 | | |
14 | 17 | | |
| |||
171 | 174 | | |
172 | 175 | | |
173 | 176 | | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
89 | 89 | | |
90 | 90 | | |
91 | 91 | | |
92 | | - | |
93 | | - | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
94 | 95 | | |
95 | 96 | | |
96 | 97 | | |
| |||
99 | 100 | | |
100 | 101 | | |
101 | 102 | | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
102 | 110 | | |
103 | 111 | | |
104 | 112 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
74 | 74 | | |
75 | 75 | | |
76 | 76 | | |
77 | | - | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
78 | 85 | | |
79 | 86 | | |
80 | 87 | | |
| |||
511 | 518 | | |
512 | 519 | | |
513 | 520 | | |
| 521 | + | |
| 522 | + | |
| 523 | + | |
| 524 | + | |
| 525 | + | |
| 526 | + | |
| 527 | + | |
| 528 | + | |
| 529 | + | |
| 530 | + | |
| 531 | + | |
| 532 | + | |
| 533 | + | |
514 | 534 | | |
515 | 535 | | |
516 | 536 | | |
| |||
541 | 561 | | |
542 | 562 | | |
543 | 563 | | |
| 564 | + | |
| 565 | + | |
| 566 | + | |
544 | 567 | | |
545 | 568 | | |
546 | 569 | | |
| |||
549 | 572 | | |
550 | 573 | | |
551 | 574 | | |
| 575 | + | |
| 576 | + | |
| 577 | + | |
552 | 578 | | |
553 | 579 | | |
554 | 580 | | |
| |||
559 | 585 | | |
560 | 586 | | |
561 | 587 | | |
| 588 | + | |
| 589 | + | |
562 | 590 | | |
563 | 591 | | |
564 | 592 | | |
| |||
0 commit comments