Skip to content

Add more safeguards when commit docs is false, support 2 level identation in frontmatter, .md files#1358

Closed
fell-lucas wants to merge 1 commit intogsd-build:mainfrom
fell-lucas:main
Closed

Add more safeguards when commit docs is false, support 2 level identation in frontmatter, .md files#1358
fell-lucas wants to merge 1 commit intogsd-build:mainfrom
fell-lucas:main

Conversation

@fell-lucas
Copy link
Copy Markdown

What

Fixes planning doc commit guardrails and makes verify key-links correctly read must_haves.key_links from valid plan frontmatter.

Why

Agents were still being guided toward staging .planning/ files outside the CLI guard, and key-links verification was failing on real plan files because the parser assumed one rigid YAML indentation shape.

Closes #1356

How

Updated the executor/workflow/reference guidance so .planning/ artifacts are never staged with raw git and must go through gsd-tools.cjs commit, which preserves the existing commit_docs and gitignore checks. Hardened parseMustHavesBlock to parse must_haves blocks relative to actual indentation instead of fixed spacing, and added regressions covering template-style 2-space YAML plus end-to-end verify key-links behavior.

Testing

Platforms tested

  • macOS
  • Windows (including backslash path handling)
  • Linux

Runtimes tested

  • Claude Code
  • Gemini CLI
  • OpenCode
  • Codex
  • Copilot
  • N/A (not runtime-specific)

Test details

Automated tests

Checklist

  • Follows GSD style (no enterprise patterns, no filler)
  • Updates CHANGELOG.md for user-facing changes
  • No unnecessary dependencies added
  • Works on Windows (backslash paths tested)
  • Templates/references updated if behavior changed
  • Existing tests pass (npm test)

Breaking Changes

None

Copy link
Copy Markdown
Collaborator

@trek-e trek-e left a comment

Choose a reason for hiding this comment

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

Adversarial Review -- PR #1358

Verdict: REQUEST CHANGES

This PR addresses a real bug (#1356) with a solid approach: replacing the rigid fixed-indent YAML parser with an indent-relative one. The documentation guardrails for commit_docs: false are reasonable. However, there are several issues that need to be addressed before merge.


1. CHANGELOG not updated (checklist discrepancy)

The PR checklist marks "Updates CHANGELOG.md for user-facing changes" as complete, but no CHANGELOG file appears in the diff. This is a user-facing bugfix (frontmatter parsing was broken for standard 2-space YAML indentation) and warrants a CHANGELOG entry.

2. stripYamlQuotes accepts mismatched quotes

function stripYamlQuotes(value) {
  return value.replace(/^['"]|['"]$/g, '');
}

This regex independently strips a leading quote and a trailing quote regardless of whether they match. Input like 'hello" becomes hello -- stripping mismatched delimiters. While valid YAML would never produce this, a defensive parser should either (a) only strip when both quotes match, or (b) document this as intentional. Suggested fix:

function stripYamlQuotes(value) {
  if ((value.startsWith("'") && value.endsWith("'")) ||
      (value.startsWith('"') && value.endsWith('"'))) {
    return value.slice(1, -1);
  }
  return value;
}

3. Test coverage gap -- verify.test.cjs dropped 4-space indent coverage

The writePlanWithKeyLinks helper was changed from 4-space to 2-space indentation, and the test was renamed to "template-style 2-space indentation." This means the end-to-end verify key-links flow is no longer regression-tested with 4-space indentation. Since existing plan files in the wild may use 4-space indentation (the previous default), both indentation styles should be tested in the verify suite. Keep the existing 4-space test and add the 2-space test alongside it rather than replacing.

4. No regression test matching the reporter's exact YAML structure

Issue #1356 shows a plan file with many top-level keys (phase, plan, type, wave, depends_on, files_modified, autonomous, requirements) preceding must_haves, and must_haves itself contains truths, artifacts, and key_links blocks. Adding a test that mirrors this exact structure would provide stronger confidence that the specific reported scenario is covered, not just a simplified approximation.

5. PR branches from fork's main -- not a blocker

The head ref is main from the contributor's fork. This is standard GitHub fork workflow and functions correctly, but using a descriptive feature branch name (e.g., fix/frontmatter-indent-parsing) would make branch management cleaner on the contributor's side.

6. No conflicts with other open PRs

PR #1289 (release strategy) modifies entirely different files (CI workflows, VERSIONING.md). No file or logic path overlap detected.


Summary of required changes before merge

  1. Add a CHANGELOG entry for the frontmatter parsing fix.
  2. Fix stripYamlQuotes to only strip matched quote pairs.
  3. Restore the 4-space indentation test in verify.test.cjs (add 2-space as a new test, do not replace).
  4. Add a regression test that mirrors the exact YAML structure from issue #1356.

Items 2-4 are code correctness and coverage issues. Item 1 is process compliance. All are straightforward to address.

@fell-lucas fell-lucas reopened this Mar 24, 2026
@fell-lucas
Copy link
Copy Markdown
Author

Hey my bad I just saw you merged another PR which fixes the issue, thanks!

@fell-lucas fell-lucas closed this Mar 24, 2026
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.

No must_haves.key_links found in frontmatter

2 participants