docs: add an Agent Skill so agents use opendataloader-pdf correctly#647
Open
hyunhee-jo wants to merge 13 commits into
Open
docs: add an Agent Skill so agents use opendataloader-pdf correctly#647hyunhee-jo wants to merge 13 commits into
hyunhee-jo wants to merge 13 commits into
Conversation
Objective: An AI coding agent asked to extract data from a PDF with opendataloader-pdf has no reliable way to get it right the first time. There are many mode/format/option combinations, and the tool often exits 0 while producing empty or wrong output — JSON to stdout prints nothing, enrichment is silently skipped without --hybrid-mode full, image-only PDFs yield no text. Agents read exit 0 as success and hand back broken results. Approach: Add a self-contained Agent Skill under skills/odl-pdf/ that walks the agent through choose-mode -> run -> VERIFY -> diagnose, with no change to ODL core code. Two choices drive the design: (1) never trust memorized flags -- the installed CLI's --help is the runtime authority, repo options.json (exported from the CLI definitions) is the checkout snapshot, version-tagged matrices are only a fallback; (2) make VERIFY a distinct step because exit 0 != success, and triage a non-zero exit by cause, with a dedicated branch for the veraPDF font crash that OCR/hybrid cannot bypass. Adds references, helper scripts, 20 behavioral evals, and drift/smoke CI. Evidence: Ran 4 fresh no-context agents, each given ONLY this skill plus a real PDF and a real task, driving the installed ODL end-to-end. - Digital paper (RAG + citations): chose local+JSON, hit a veraPDF font crash, re-ran verbose, reported it as an upstream bug -- did NOT return empty output as success. - Table-heavy PDF: chose local, VERIFIED tables, escalated to --table-method cluster and recovered the second table, flagged hybrid as the next lever; reported the merged-column limitation honestly. - Scanned CJK PDF: detected image-only, hit the same font crash, proved OCR/hybrid could not bypass it (the crash precedes page triage). - "JSON to stdout | jq": reproduced the 0-byte empty-output trap the skill predicts, then delivered a working file-mode alternative. All four followed the skill without inventing flags. Internal-consistency checks (drift 31=31, evals schema, cross-platform script smoke) also pass but are not the evidence -- the dogfood is. Refreshes and supersedes #395 (close on merge).
hyunhee-jo
requested review from
LonelyMidoriya,
MaximPlusov,
bundolee and
hnc-jglee
as code owners
July 20, 2026 08:07
Contributor
WalkthroughAdds the ChangesODL PDF skill
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant detect-env.sh
participant opendataloader-pdf
participant hybrid-health.sh
participant verify-json.py
participant quick-eval.py
Agent->>detect-env.sh: discover runtime and installation state
Agent->>hybrid-health.sh: check hybrid server health
Agent->>opendataloader-pdf: extract PDF content with selected options
Agent->>verify-json.py: summarize JSON output
Agent->>quick-eval.py: compare extracted text with ground truth
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/skill-smoke-test.yml:
- Around line 29-36: Add a top-level concurrency configuration to both
workflows: .github/workflows/skill-smoke-test.yml lines 29-36 and
.github/workflows/skill-drift-check.yml lines 23-25. Use the workflow-and-ref
group expression and enable cancel-in-progress so newer runs replace redundant
pending runs in each workflow.
- Around line 44-46: Pin the actions/checkout and actions/setup-python
references in .github/workflows/skill-smoke-test.yml lines 44-46 and
.github/workflows/skill-drift-check.yml lines 27-29 to full commit SHAs,
optionally retaining the version as a trailing comment; update both workflows
consistently.
In `@skills/odl-pdf/references/integration-examples.md`:
- Around line 150-155: Update the chunk-building logic around the buffer flush
and append operations to enforce max_chars for individual oversized text
elements: split any text longer than max_chars into bounded segments, preserving
the same citation metadata for each segment, while retaining the existing
aggregation behavior for normal elements.
- Around line 287-295: Update the remote-server command examples around
opendataloader-pdf-hybrid to replace angle-bracket placeholders such as
<PRIVATE_BIND_ADDRESS> and <gpu-host> with shell-safe quoted environment
variables or concrete addresses. Keep the documented host, port, hybrid mode,
URL, and timeout behavior unchanged while ensuring the examples are
copy-pasteable.
- Around line 142-143: Update chunk_with_citations to open the JSON file with a
context manager and load it through the managed file handle, preserving the
existing json_path input and resulting doc value while ensuring the handle is
closed after reading.
In `@skills/odl-pdf/references/options-matrix.md`:
- Around line 217-229: Update the AI-enriched extraction command in the
“AI-enriched extraction (hybrid mode)” section to include the server-side
enrichment flags required for formula OCR and picture descriptions:
--enrich-formula and --enrich-picture-description. Keep the existing hybrid
backend, full mode, URL, and no-fallback guidance unchanged.
- Around line 112-122: Update the “--hybrid requires a running server” section
to qualify the failure statement when --hybrid-fallback is enabled: explain that
requests may complete through the local Java path if the hybrid server is
unreachable, but the output is fallback output rather than successful hybrid
extraction. Preserve the existing guidance for non-fallback configurations and
server startup.
In `@skills/odl-pdf/scripts/sync-skill-refs.py`:
- Around line 178-183: Remove the unnecessary f-string prefixes from the static
print messages in the options-reporting logic, including the “NEW options” and
“REMOVED options” headers. Keep interpolation unchanged for the loop output that
uses the name variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f7b1a5a4-c2ae-4daf-96ef-22a2fd684f8d
📒 Files selected for processing (17)
.github/workflows/skill-drift-check.yml.github/workflows/skill-smoke-test.ymlskills/README.mdskills/odl-pdf/README.mdskills/odl-pdf/SKILL.mdskills/odl-pdf/evals/evals.jsonskills/odl-pdf/references/eval-metrics.mdskills/odl-pdf/references/format-guide.mdskills/odl-pdf/references/hybrid-guide.mdskills/odl-pdf/references/installation-matrix.mdskills/odl-pdf/references/integration-examples.mdskills/odl-pdf/references/options-matrix.mdskills/odl-pdf/scripts/detect-env.shskills/odl-pdf/scripts/hybrid-health.shskills/odl-pdf/scripts/quick-eval.pyskills/odl-pdf/scripts/sync-skill-refs.pyskills/odl-pdf/scripts/verify-json.py
…racy, F541
Objective: A public reviewer (CodeRabbit) flagged copy-paste examples that break
in a real shell plus a few doc/lint issues in the skill's reference files.
Approach: Fix the verified findings; decline two by repo convention.
- integration-examples.md: the remote-server example used <ANGLE_BRACKET>
placeholders (a shell parses "<" as input redirection, so the "copy-pasteable"
command failed before ODL ran) -> quoted env var; the JSON reader now uses a
context manager; and a note documents that max_chars bounds size BETWEEN
elements (a single element larger than max_chars is not split).
- options-matrix.md: qualify "a hybrid server is required" for --hybrid-fallback
(the run then completes via local Java as fallback output, not hybrid); and note
that formula/picture enrichment needs the hybrid SERVER's --enrich-* flags, not
just the client's --hybrid-mode full (CodeRabbit's proposed client-flag fix was
wrong -- those are server startup flags).
- sync-skill-refs.py: drop f-prefixes on placeholder-free strings (Ruff F541).
Declined: pinning actions to commit SHAs (repo convention is floating tags -- the
sibling workflows use actions/checkout@v7 / setup-python@v6 unpinned, so pinning
only these two would be inconsistent) and adding a concurrency group (CodeRabbit
itself marked it low value for these light jobs).
Evidence: ran the fixed code, did not just edit it.
- Extracted the fixed chunk_with_citations from the reference and ran it on real
ODL JSON: the context-manager path works and chunks carry {page,bbox}; a
5000-char element with max_chars=1000 becomes its own oversized chunk (matches
the new note).
- Shell-safety before/after: old form -> "bash: PRIVATE_BIND_ADDRESS: No such
file or directory"; fixed form resolves to --host 10.0.0.5.
- sync-skill-refs.py still runs (drift 31=31, exit 0); f-prefixes gone; py_compile OK.
- skill-gate PASS (fences, evals schema, cross-refs, compile, drift).
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/odl-pdf/references/integration-examples.md (1)
151-156: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude inserted newline separators in the
max_charscalculation.The buffer counts only element text, but the output adds
"\n"between elements. For example, two 500-character elements produce a 1001-character chunk withmax_chars=1000, contradicting the documented bound. Track the separator length when deciding whether to flush.Proposed fix
- if buf_len + len(text) > max_chars and buf: + separator_len = 1 if buf else 0 + if buf_len + separator_len + len(text) > max_chars and buf: chunks.append({"text": "\n".join(t for t, _ in buf), "citations": [m for _, m in buf]}) buf, buf_len = [], 0 buf.append((text, meta)) - buf_len += len(text) + buf_len += (1 if len(buf) > 1 else 0) + len(text)Also applies to: 164-166
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/odl-pdf/references/integration-examples.md` around lines 151 - 156, The chunking logic must account for newline separators added by "\n". Update the buffer length calculation and flush condition around buf, buf_len, and max_chars so each additional element includes one separator character, while preserving the existing append and citation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@skills/odl-pdf/references/integration-examples.md`:
- Around line 151-156: The chunking logic must account for newline separators
added by "\n". Update the buffer length calculation and flush condition around
buf, buf_len, and max_chars so each additional element includes one separator
character, while preserving the existing append and citation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6bbfc3a8-4914-4dcf-bc9b-7ac3fc6f220b
📒 Files selected for processing (3)
skills/odl-pdf/references/integration-examples.mdskills/odl-pdf/references/options-matrix.mdskills/odl-pdf/scripts/sync-skill-refs.py
…ax_chars Objective: The reference RAG chunker advertises a max_chars bound, but the size accounting summed only the raw element text and ignored the "\n" that join() inserts between elements. So combining normal-size elements could exceed the bound (two 500-char elements at max_chars=1000 produced a 1001-char chunk), which also contradicted the note that says only a single oversized element can exceed it. Approach: Count the joining separator (1 char when the buffer is non-empty) in both the flush threshold and the running length, resetting it after a flush. Elements are still not split, so a single element longer than max_chars remains its own oversized chunk — which is exactly what the note documents. Evidence: ran the extracted chunker before/after. - two 500-char, max 1000: before = one 1001-char chunk; after = two chunks, max 500. - three 400-char, max 1000: after = [801, 400], all within 1000 (801 = 400+1+400). - single 5000-char: still one oversized chunk (note stays accurate). - citations preserved; skill-gate PASS.
…t categorized sources (tier 1)
…on values vs options.json
…drops the option inventory table
… order; option-interactions cross-refs
… new discovery-source fallback (no bundled snapshot)
… fail-on-bad-token (no network)
…(exit 2); align CI branch + message; strip internal labels; authority table 3rd row
…on defaults vs options.json (catches default-flips)
…ase-review checklist for version-specific behaviors (+ deferred behavioral-eval-in-CI note)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/odl-pdf/README.md (1)
31-38: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the drift-check helper in the contents table.
The table lists the runtime helpers but omits
scripts/sync-skill-refs.py, even though the maintainer note identifies it as the CI drift-check utility. Add it so maintainers can discover the referenced workflow directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/odl-pdf/README.md` around lines 31 - 38, Update the contents table in README.md to include scripts/sync-skill-refs.py alongside the other scripts listed in the scripts row, identifying it as the CI drift-check helper referenced by the Maintainer note.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/odl-pdf/SKILL.md`:
- Around line 86-91: Update the Stage 1 option-discovery flow so a
repository-root options.json snapshot is consulted before the homepage CLI
Options Reference whenever the CLI is unavailable. Preserve the existing
installed-CLI commands as the first choice, use
references/installation-matrix.md for installation guidance when needed, and
only fall back to the version-caveated homepage reference when no local snapshot
is available.
---
Outside diff comments:
In `@skills/odl-pdf/README.md`:
- Around line 31-38: Update the contents table in README.md to include
scripts/sync-skill-refs.py alongside the other scripts listed in the scripts
row, identifying it as the CI drift-check helper referenced by the Maintainer
note.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 12083238-e56e-467f-9580-8b1c756d0f7c
📒 Files selected for processing (9)
.github/workflows/skill-drift-check.yml.github/workflows/skill-smoke-test.ymlskills/odl-pdf/MAINTAINING.mdskills/odl-pdf/README.mdskills/odl-pdf/SKILL.mdskills/odl-pdf/evals/evals.jsonskills/odl-pdf/references/format-guide.mdskills/odl-pdf/references/option-interactions.mdskills/odl-pdf/scripts/sync-skill-refs.py
Comment on lines
+86
to
+91
| Then, if ODL is installed, capture the real option surface: | ||
| `opendataloader-pdf --help` (and `opendataloader-pdf-hybrid --help` if OCR or | ||
| enrichment is in play). If not installed yet, load | ||
| `references/installation-matrix.md` for install guidance; until the CLI is | ||
| runnable, get the option surface from the homepage CLI Options Reference | ||
| (version-caveated per "Version & option authority" above). |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor the repository snapshot before using the public reference.
The discovery order in lines 50–60 says to use repo-root options.json when no CLI is available, but this Stage 1 branch sends the agent directly to the homepage reference. In a repository checkout, that can produce a stale provisional command despite a local snapshot being available.
Proposed wording
-If not installed yet, load
-`references/installation-matrix.md` for install guidance; until the CLI is
-runnable, get the option surface from the homepage CLI Options Reference
-(version-caveated per "Version & option authority" above).
+If not installed yet, load `references/installation-matrix.md` for install
+guidance. If a repo checkout is available, use its root `options.json` as the
+provisional option surface; otherwise use the homepage CLI Options Reference
+(version-caveated per "Version & option authority" above).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Then, if ODL is installed, capture the real option surface: | |
| `opendataloader-pdf --help` (and `opendataloader-pdf-hybrid --help` if OCR or | |
| enrichment is in play). If not installed yet, load | |
| `references/installation-matrix.md` for install guidance; until the CLI is | |
| runnable, get the option surface from the homepage CLI Options Reference | |
| (version-caveated per "Version & option authority" above). | |
| Then, if ODL is installed, capture the real option surface: | |
| `opendataloader-pdf --help` (and `opendataloader-pdf-hybrid --help` if OCR or | |
| enrichment is in play). If not installed yet, load | |
| `references/installation-matrix.md` for install guidance. If a repo checkout is | |
| available, use its root `options.json` as the provisional option surface; | |
| otherwise use the homepage CLI Options Reference (version-caveated per "Version | |
| & option authority" above). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/odl-pdf/SKILL.md` around lines 86 - 91, Update the Stage 1
option-discovery flow so a repository-root options.json snapshot is consulted
before the homepage CLI Options Reference whenever the CLI is unavailable.
Preserve the existing installed-CLI commands as the first choice, use
references/installation-matrix.md for installation guidance when needed, and
only fall back to the version-caveated homepage reference when no local snapshot
is available.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
An AI coding agent asked to get data out of a PDF with opendataloader-pdf — plain text, Markdown, structured JSON with tables and bounding boxes, a RAG pipeline with page-level citations, or OCR for a scanned document — has no reliable way to do it correctly the first time. ODL has many mode/format/option combinations, and it frequently exits 0 while producing empty or wrong output (JSON to stdout prints nothing; enrichment is silently skipped without
--hybrid-mode full; image-only PDFs yield no text). Agents read exit 0 as success and return broken results.Refreshes and supersedes #395 (close on merge).
Approach
Add a self-contained Agent Skill under
skills/odl-pdf/that guides an agent through choose-mode → run → VERIFY → diagnose, with no change to ODL core code. Two design choices:--helpis the runtime authority; repooptions.json(exported from the CLI's option definitions) is the checkout snapshot; version-tagged reference matrices are only a fallback. The skill never puts an unconfirmed option into a generated command.17 files, additions only.
Contents (what each part is)
Progressive disclosure: an agent reads
SKILL.mdfirst and loads a reference or runs a script only when a step needs it.skills/README.md— index of theskills/directory: what skills exist and how to enable them.skills/odl-pdf/SKILL.md— the agent instruction sheet: a 7-stage workflow (trigger → environment discovery → DECIDE mode+format → EXECUTE → VERIFY → DIAGNOSE → RAG handoff) plus the option-authority chain, critical gotchas, and safety rules.skills/odl-pdf/README.md— human-facing overview.references/(loaded on demand):installation-matrix,options-matrix,hybrid-guide,format-guide,integration-examples(CLI/Python/Node/LangChain/LlamaIndex/Java + JSON citation chunking),eval-metrics.scripts/(helpers the skill runs):detect-env.sh,hybrid-health.sh,verify-json.py(VERIFY summary),quick-eval.py,sync-skill-refs.py(CI option-drift check).evals/evals.json— 20 behavioral evals (decision-correctness oracle) with a frozen scoring contract..github/workflows/—skill-drift-check.ymlandskill-smoke-test.yml, both least-privilegecontents: read.Evidence
Ran 4 fresh no-context agents, each given only the skill + a real PDF + a real task, driving the installed ODL end-to-end:
--table-method cluster, recovered the 2nd table; flagged hybrid as next; reported merged-column limit honestly--format json --to-stdout | jqAll four followed the skill without inventing flags. Internal-consistency checks (drift 31=31, evals schema valid, cross-platform script smoke) also pass, but the dogfood — not those checks — is the evidence that the skill solves the agent's problem.
Validated against ODL 2.5.0.
Summary by CodeRabbit
New Features
Documentation
Tests