Skip to content

fix(integrations): stop frontmatter injection gluing onto a missing trailing newline - #4570

Open
Noor-ul-ain001 wants to merge 2 commits into
github:mainfrom
Noor-ul-ain001:fix/frontmatter-flag-no-trailing-newline
Open

Noor-ul-ain001 wants to merge 2 commits into
github:mainfrom
Noor-ul-ain001:fix/frontmatter-flag-no-trailing-newline

Conversation

@Noor-ul-ain001

Copy link
Copy Markdown
Contributor

Summary

_inject_frontmatter_flag() in ClaudeIntegration/VibeIntegration/AlquimiaAIIntegration detects the closing --- line's existing EOL and reuses it when injecting a new frontmatter key. When that --- is the file's last line with no trailing newline, the injected text is appended with no newline separator at all, producing user-invocable: true--- instead of a clean, separately-lined ---.

This corrupts the frontmatter (the closing delimiter is no longer alone on its own line), and since post_process_skill_content() chains multiple calls (user-invocable, then disable-model-invocation, then any fork-context keys), the second call's pre-scan can no longer find a second --- line at all — so every subsequent key injection is silently dropped, not just corrupted.

post_process_skill_content() runs on content from "external skill generators (presets, extensions)" per its own docstring, so a trailing newline after the closing delimiter isn't guaranteed.

>>> ClaudeIntegration._inject_frontmatter_flag("---\nname: x\n---", "user-invocable")
'---\nname: x\nuser-invocable: true---'          # corrupted delimiter
>>> # a second call on that output:
>>> ClaudeIntegration._inject_frontmatter_flag(_, "disable-model-invocation", "false")
'---\nname: x\nuser-invocable: true---'           # unchanged -- silently dropped

DroidIntegration's own copy of this exact helper already emits an unconditional \n instead of detecting/reusing the existing EOL, avoiding this bug entirely. Ported that fix to the other three implementations that share the same code.

Changes

  • src/specify_cli/integrations/claude/__init__.py, vibe/__init__.py, alquimia/__init__.py: _inject_frontmatter_flag now always emits f"{key}: {value}\n", matching DroidIntegration.
  • Added a regression test to each affected integration's test file, covering both the single-call corruption and the chained-calls silent-drop.

Test plan

  • ruff check . clean
  • New tests fail against the pre-fix code (verified via test-the-test) and pass with the fix
  • Full tests/integrations/test_integration_{claude,vibe,alquimia,droid}.py suite: 185 passed, 1 pre-existing/unrelated skip

…railing newline

ClaudeIntegration/VibeIntegration/AlquimiaAIIntegration's
_inject_frontmatter_flag() detected the closing "---" line's existing
EOL and reused it when injecting a new key -- so when that "---" was
the file's last line with no trailing newline, the injected text was
appended with no newline at all, producing "user-invocable: true---"
instead of a properly separated line. This corrupts the frontmatter
(the closing delimiter is no longer alone on its own line) and, since
post_process_skill_content() chains multiple injection calls, silently
drops every subsequent key: a second call's pre-scan can no longer find
a second "---" line to inject before, so e.g.
"disable-model-invocation: false" is never added at all.

post_process_skill_content() runs on content from "external skill
generators (presets, extensions)" per its own docstring, so a trailing
newline after the closing delimiter isn't guaranteed.

DroidIntegration's own copy of this helper already emits an
unconditional "\n" instead of detecting/reusing the existing EOL,
exactly avoiding this bug -- ported that fix to the other three
implementations. Added a regression test to each of the three affected
integrations' test files, covering both the single-call corruption and
the chained-calls silent-drop.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U74yBbvVQCPwB7Ed8Dzeu6
@mnriem mnriem added the triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review label Sep 14, 2026
@mnriem

mnriem commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

The header-only reproduction demonstrates the helper defect. Could you show a supported preset or extension rendering path that produces this input? A skill containing a body does not exhibit this failure merely because its final newline is missing, so we’re keeping this as triage-can-wait pending that evidence.

Please also preserve existing line endings: the new unconditional LF introduces mixed endings into CRLF content. Retain the detected line ending when present and supply a separator when it is absent.

Please complete the AI disclosure with the tool, mode/settings, and extent of assistance; the Claude Sonnet 5 attribution is already present in the commit.

Drafted for @mnriem with assistance from GitHub Copilot (model: GPT-6 Astra; interactive comment drafting).

@mnriem
mnriem requested a balanced review from Copilot September 16, 2026 12:30
@mnriem mnriem added triage-can-wait Verdict: valid and in-scope but deprioritized; held behind the evidence gate author-needs-disclosure AI use, or the agent/model/settings behind it, not disclosed per CONTRIBUTING author-awaiting Waiting on author response author-needs-info Missing detail needed to assess — supply requested info and removed triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review labels Sep 16, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unconditional LF insertion produces mixed line endings for CRLF content in all three implementations.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes malformed skill frontmatter when the closing delimiter lacks a trailing newline.

Changes:

  • Adds separator newlines in three integrations.
  • Adds direct and chained regression tests.
  • Static review found CRLF preservation regressions.
File summaries
File Description
src/specify_cli/integrations/claude/__init__.py Updates frontmatter injection.
src/specify_cli/integrations/vibe/__init__.py Updates frontmatter injection.
src/specify_cli/integrations/alquimia/__init__.py Updates frontmatter injection.
tests/integrations/test_integration_claude.py Adds regression tests.
tests/integrations/test_integration_vibe.py Adds regression tests.
tests/integrations/test_integration_alquimia.py Adds regression tests.
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

else:
eol = ""
out.append(f"{key}: {value}{eol}")
out.append(f"{key}: {value}\n")
else:
eol = ""
out.append(f"{key}: {value}{eol}")
out.append(f"{key}: {value}\n")
else:
eol = ""
out.append(f"{key}: {value}{eol}")
out.append(f"{key}: {value}\n")
…lags

Per Copilot review on PR github#4570: unconditionally emitting "\n" after the
injected key regressed CRLF-authored skills into mixed line endings.
Detect the closing delimiter's existing EOL (\r\n or \n) and only fall
back to "\n" when the delimiter has none at all (the original
no-trailing-newline corruption bug).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-awaiting Waiting on author response author-needs-disclosure AI use, or the agent/model/settings behind it, not disclosed per CONTRIBUTING author-needs-info Missing detail needed to assess — supply requested info triage-can-wait Verdict: valid and in-scope but deprioritized; held behind the evidence gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants