Skip to content

fix(config): resolve dotted YAML paths to direct children only#680

Open
JSap0914 wants to merge 1 commit into
fathah:mainfrom
JSap0914:fix-getyamlpath-strict-paths
Open

fix(config): resolve dotted YAML paths to direct children only#680
JSap0914 wants to merge 1 commit into
fathah:mainfrom
JSap0914:fix-getyamlpath-strict-paths

Conversation

@JSap0914

Copy link
Copy Markdown

Bug

getConfigValue reads config.yaml through getYamlPath, but that walker is too
permissive on dotted paths. It reset its depth to the matched-stack size on every
line, so when an intermediate path segment wasn't itself on the stack, the next
segment matched at any deeper indent. Two concrete consequences:

  • agent.service_tier resolves a grandchild — e.g. agent.fallback.service_tier
    instead of returning null when there is no direct agent.service_tier.
  • A flat key like service_tier matches the first nested occurrence in any block
    (e.g. under telegram: or agent:) instead of resolving only at the top level.

The write path (setConfigValuefindYamlPath) already enforces direct-child
depth and pins single-segment keys to column 0; the read path didn't, so reads and
writes disagreed. This is the follow-up the repo's own tests/config-value-paths.test.ts
flagged with three it.skip cases (originating from #247/#243).

Fix

Rewrite getYamlPath to walk one segment at a time and match each segment only
among the direct children of its parent block (the shallowest non-blank indent
inside that block), skipping deeper grandchildren. Single-segment paths use a
parent indent of -1, so only column-0 keys match. No behavior change for the
existing well-formed lookups; this only tightens the over-matching cases.

Un-skips the three regression tests that documented the desired strictness.

Verification

npx vitest run tests/config-value-paths.test.ts tests/yaml-path.test.ts tests/auxiliary-config.test.ts tests/memory-limits.test.ts

Before the fix the three un-skipped cases failed (e.g. expected 'nested-deeper' to be null).
After: Test Files 4 passed (4), Tests 53 passed (53). eslint src/main/yaml-path.ts is clean.

getConfigValue reads via getYamlPath, whose walker reset its depth to the
matched-stack size on every line. When an intermediate segment of a dotted
path was not itself on the stack, the next segment matched at any deeper
indent, so:

  - agent.service_tier matched a grandchild (agent.fallback.service_tier).
  - a flat key like service_tier matched the first nested occurrence in any
    block instead of resolving only at the top level.

The write path (setConfigValue via findYamlPath) already enforced
direct-child depth and column-0 flat keys; this aligns the read path with it.
Rewrite getYamlPath to walk one segment at a time, matching each segment only
among the direct children of its parent block (the shallowest indent inside
the block) and pinning single-segment keys to column 0.

Un-skips the three regression tests in config-value-paths.test.ts that
documented this gap.
Copilot AI review requested due to automatic review settings June 16, 2026 02:52

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates YAML path resolution strictness and enables previously skipped tests to verify correct dotted/flat-key behavior.

Changes:

  • Enforces “direct-child only” matching for dotted paths in getYamlPath
  • Pins flat (single-segment) keys to the top level and unskips corresponding tests
  • Converts previously skipped tests into active assertions for grandchildren/nested occurrences

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
tests/config-value-paths.test.ts Unskips tests and documents intended strict YAML path behavior
src/main/yaml-path.ts Reworks getYamlPath scanning to enforce direct-child depth and top-level-only flat keys

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/main/yaml-path.ts
Comment on lines +31 to +37
// Walk the path one segment at a time. `searchStart` is the first line to
// scan for the current segment; `parentIndent` bounds the parent's block —
// children live at indent strictly greater than it. The first segment uses
// parentIndent = -1, so only column-0 keys match (a flat/single-segment key
// is pinned to the top level and never resolves a nested occurrence).
let searchStart = 0;
let parentIndent = -1;
Comment thread src/main/yaml-path.ts
// without finding the next part), so reset pathIdx to the depth we are
// actually at — i.e. number of parts already matched in stack.
pathIdx = stack.length;
for (let p = 0; p < parts.length; p++) {
Comment thread src/main/yaml-path.ts
Comment on lines +47 to +48
let i = searchStart;
for (; i < lines.length; i++) {
@greptile-apps

greptile-apps Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Rewrites getYamlPath (the YAML read path in src/main/yaml-path.ts) to walk the dotted key one segment at a time, matching each segment only among the direct children of its parent block and pinning single-segment keys to column 0 — bringing the read path into alignment with the already-strict findYamlPath write path.

  • Core change: replaces a stack-pop loop with a two-pointer scan (searchStart, parentIndent) plus a directChildIndent sentinel that skips any line at an indent deeper than the shallowest child level, preventing grandchild and cross-block false matches.
  • Test activation: three it.skip cases in tests/config-value-paths.test.ts that documented the desired strictness (grandchild depth, flat-key column-0, cross-sibling matching) are un-skipped and all pass.

Confidence Score: 5/5

Safe to merge — the rewrite is a targeted tightening of the read path with no behavioral change for well-formed lookups, and all 53 tests pass including the three previously-skipped regression cases.

The new two-pointer algorithm is logically correct across all traced paths: grandchild skipping via directChildIndent, column-0 pinning via parentIndent = -1, and block-boundary exit via indent <= parentIndent all behave as intended. The as number non-null assertion on line 77 is provably safe because descendInto >= 0 (guaranteed by the guard two lines above) can only be reached after directChildIndent was already assigned. Existing passing tests are unaffected, and the three newly-enabled tests directly cover the bugs described in the PR.

No files require special attention.

Important Files Changed

Filename Overview
src/main/yaml-path.ts Rewrites getYamlPath to walk one segment at a time and enforce direct-child depth via a directChildIndent sentinel, fixing over-permissive grandchild and flat-key matching. Logic is correct; the final return null after the loop is unreachable but required by TypeScript's return-type checker.
tests/config-value-paths.test.ts Un-skips three previously documented regression tests (grandchild depth, flat-key column-0, and cross-sibling matching) that now pass with the new implementation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([getYamlPath called]) --> B{parts.length == 0?}
    B -- yes --> Z1([return null])
    B -- no --> C[searchStart=0\nparentIndent=-1]
    C --> D[for p=0..parts.length-1]
    D --> E[directChildIndent=null\ndescendInto=-1\ni=searchStart]
    E --> F{i < lines.length?}
    F -- no --> G
    F -- yes --> H[read line i]
    H --> I{blank or comment?}
    I -- yes --> F2[i++] --> F
    I -- no --> J{indent <= parentIndent?}
    J -- yes --> G[after inner loop]
    J -- no --> K{directChildIndent not set?}
    K -- yes --> L[directChildIndent = indent]
    K -- no --> M
    L --> M{indent == directChildIndent?}
    M -- no grandchild --> F2
    M -- yes direct child --> N{key matches parts p?}
    N -- no --> F2
    N -- yes --> O{isLeaf?}
    O -- yes --> P([return parseScalar])
    O -- no intermediate --> Q[descendInto = i, break]
    Q --> G
    G --> R{isLeaf OR descendInto lt 0?}
    R -- yes path missing --> Z2([return null])
    R -- no --> S[searchStart = descendInto+1\nparentIndent = directChildIndent]
    S --> D
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A([getYamlPath called]) --> B{parts.length == 0?}
    B -- yes --> Z1([return null])
    B -- no --> C[searchStart=0\nparentIndent=-1]
    C --> D[for p=0..parts.length-1]
    D --> E[directChildIndent=null\ndescendInto=-1\ni=searchStart]
    E --> F{i < lines.length?}
    F -- no --> G
    F -- yes --> H[read line i]
    H --> I{blank or comment?}
    I -- yes --> F2[i++] --> F
    I -- no --> J{indent <= parentIndent?}
    J -- yes --> G[after inner loop]
    J -- no --> K{directChildIndent not set?}
    K -- yes --> L[directChildIndent = indent]
    K -- no --> M
    L --> M{indent == directChildIndent?}
    M -- no grandchild --> F2
    M -- yes direct child --> N{key matches parts p?}
    N -- no --> F2
    N -- yes --> O{isLeaf?}
    O -- yes --> P([return parseScalar])
    O -- no intermediate --> Q[descendInto = i, break]
    Q --> G
    G --> R{isLeaf OR descendInto lt 0?}
    R -- yes path missing --> Z2([return null])
    R -- no --> S[searchStart = descendInto+1\nparentIndent = directChildIndent]
    S --> D
Loading

Reviews (1): Last reviewed commit: "fix(config): resolve dotted YAML paths t..." | Re-trigger Greptile

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.

2 participants