fix: parse Google docstrings missing the blank line before a section#3832
Conversation
generate_func_documentation feeds inspect.getdoc(func) to griffe's Google
parser. When a summary line is immediately followed by an Args:/Returns:/...
section with no blank line in between, griffe skips the section ("Missing
blank line above section"), so every parameter description is dropped and the
raw Args: block leaks into the function description. This is a Google-only gap;
the numpy and sphinx parsers already tolerate the missing blank line.
Normalize the docstring before parsing: when the resolved style is google,
insert a single blank line before a recognized section header that directly
follows non-indented text and is itself followed by an indented block. The
anchor mirrors griffe's own recognition (column-0 header, case-insensitive
section keys, indented line below), and the helper returns the original string
unchanged when no insertion is needed, so well-formed docstrings stay
byte-identical.
seratch
left a comment
There was a problem hiding this comment.
The underlying issue is valid: on the released path, a summary immediately followed by Args: silently drops parameter descriptions and leaves the raw block in the tool description.
Please narrow the normalization to the parameter-section aliases consumed by generate_func_documentation (Args, Arguments, Params, and Parameters), and update the generalized comments and tests accordingly. The current header list is only a partial duplicate of Griffe's section registry, while the multi-section test does not actually prove that the later Returns: section was parsed because later text sections are ignored.
Please retain the end-to-end base-failing regression and the well-formed-docstring control. After that focused change and green CI, this should be ready for another review.
Per review feedback, I narrowed the blank-line normalization to the four parameter-section aliases that generate_func_documentation actually consumes (Args, Arguments, Params, Parameters) instead of mirroring the full griffe section registry, and rescoped the comments and helper docstring accordingly. I also removed the multi-section test: its Returns: assertion passed vacuously because the helper never fires after an indented line and later text sections are ignored anyway. The base-failing regression test, the equivalence test, the well-formed control test, and the end-to-end function_tool test are retained unchanged.
|
Thanks for the review, @seratch , all three addressed in 9654627:
Locally, pytest tests/test_function_schema.py tests/test_doc_parsing.py tests/test_function_tool.py passes (83) and ruff is clean. CI looks like it's waiting on workflow approval for this first PR, happy to address anything once it runs. Ready for another look. |
Summary
A Google-style docstring whose summary line is immediately followed by an
Args:section with no blank line in between silently loses every parameter description, and the rawArgs:block leaks into the function/tool description.Before this change:
params_json_schema["properties"]["city"]has nodescriptiontool.descriptionbecomes"Get the weather for a city.\nArgs:\n city: ...\n units: ..."(the raw block)So the model never sees any parameter description and gets a malformed tool description. Adding the (PEP 257 / Google-style) blank line fixes it, but the failure is completely silent, which makes it easy to ship a degraded tool without noticing.
Root cause.
generate_func_documentationinsrc/agents/function_schema.pypassesinspect.getdoc(func)to griffe's Google parser. griffe skips a section header when there is no blank line above it and the next line is indented, loggingPossible section skipped, reasons: Missing blank line above section; the header and its body are appended to the summary text instead of becoming a parameters section. The numpy and sphinx parsers already tolerate the missing blank line, so this is a Google-only gap.Fix. When the resolved style is
google, normalize the docstring before building the griffeDocstring: insert a single blank line before a recognized section header (Args:/Returns:/Raises:/… as an exactHeader:line) that directly follows non-indented top-level text (the summary) and is itself followed by an indented block. The anchor mirrors griffe's own recognition — column-0 header, case-insensitive section keys, indented line below — so inline mentions (see Args: below) and headers already preceded by a blank line or an indented body are left untouched. The helper returns the original string object unchanged when no insertion is needed, so well-formed docstrings stay byte-identical.This is not the same as the closed PR #3795. That PR addressed a different variant (a docstring with no summary that opens directly with
Args:), and its approach (prepending"\n") was correctly closed by @seratch as a no-op because griffe appliesinspect.cleandoc()internally, which strips the leading newline. This PR targets the distinctMissing blank line above sectionskip path (a header mid-docstring, after a non-empty summary) and demonstrates a real base failure. Out of scope and deliberately left unchanged: admonitions (Note:/Warning:), headers with trailing title text (Examples: foo), and griffe's separateExtraneous blank line below section titleskip reason.Test plan
Added five tests to
tests/test_function_schema.pycoveringfunction_schema,generate_func_documentation, and the end-to-end@function_toolpath, plus a multi-section case and a byte-identity control for the already-well-formed docstring.Running the new tests on the base (before the
src/agents/function_schema.pychange) reproduces the bug (griffe logsDEBUG:griffe:<module>: Possible section skipped, reasons: Missing blank line above section):(The one passing test is the byte-identity control on the well-formed docstring, which correctly parses on base too.)
After the fix:
make format-checkandmake lintare clean;mypyandpyrightpass on the changed source file..agents/skills/code-change-verification/scripts/run.shrunsmake format(clean),make lint(pass), and the fullmake testssuite (pass).make typecheckreports 6 errors, but all of them are pre-existing on the base branch in files this PR does not touch (src/agents/sandbox/session/archive_ops.pyand two test modules that depend on an optionalany_llmstub / a newer-Pythoneager_task_factory); I verified they are unchanged from base and unrelated to this change.Validated against the locked
griffelib==2.0.1(griffelib>=2, <3).Disclosure: I used an AI coding assistant while investigating and preparing this change; I reviewed the diff, ran the tests, and stand behind the result.
Issue number
No linked issue — self-discovered while reading
function_schema.py(same discovery context as #3795, but a different, actually-failing variant).Checks
.agents/skills/code-change-verification/scripts/run.shmake typecheckerrors documented above)/reviewbefore submitting this PR