docs(agents): add source-text-analysis rule (use parsers, not regex)#89
Merged
docs(agents): add source-text-analysis rule (use parsers, not regex)#89
Conversation
Two recent fixes converged on the same architectural answer for text inspection on Ricky source-code hot paths: - #86 (`auto-fix-loop.ts:hasRickyWorkflowAliasImport`) — substring match for `from 'node:fs'` was fooled by the literal text inside a HEREDOC embedded in a `.step({ command: ... })` body. Fixed by walking `ts.createSourceFile` ImportDeclaration nodes. - #88 (`spec-intake/markdown-target-files.ts:extractTargetFilesFromMarkdown`) — regex `PATH_PATTERN` matched paths inside fenced code blocks and prose noise. Fixed by walking `mdast-util-from-markdown` inlineCode nodes. Both fixes also independently rejected LLM-based detection for the same reasons: non-determinism breaks the offline eval suite, latency multiplies on every retry, and model output is a prompt-injection surface for paths that drive shell commands. Capture this as a durable rule in AGENTS.md so the next person tempted to grep-and-substitute on TS source has the prior art. Adds a new top-level "Ricky Source Code Conventions" section above the existing workflow-authoring conventions, since it applies to Ricky's own source rather than to the workflows Ricky generates. CLAUDE.md auto-follows via the existing symlink — no separate Claude rule needed (consistent with AGENTS.md:206). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a "Ricky Source Code Conventions" section to AGENTS.md establishing rules for how Ricky inspects and modifies its own source code. The section prioritizes grammar-aware parsers (TypeScript AST, mdast) over regex and substring heuristics, defines a strict fallback strategy, and documents motivating bug patterns. ChangesRicky Source Code Conventions
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+195
to
+198
| **Concrete tools available.** Both are already in `dependencies`: | ||
|
|
||
| - TypeScript / JavaScript: `import ts from 'typescript'`; `ts.createSourceFile(name, content, ts.ScriptTarget.Latest, false, ts.ScriptKind.TS)` then walk `sourceFile.statements`. | ||
| - Markdown: `import { fromMarkdown } from 'mdast-util-from-markdown'`; walk the resulting mdast tree (`inlineCode`, `code`, `heading`, `list`, etc.). |
Contributor
There was a problem hiding this comment.
🟡 AGENTS.md falsely claims mdast-util-from-markdown is already in dependencies
Line 195-198 states "Concrete tools available. Both are already in dependencies" and then lists mdast-util-from-markdown as an available markdown parser. However, mdast-util-from-markdown is not present in package.json (neither in dependencies nor devDependencies) and is not installed in node_modules. Only typescript is actually available. Since AGENTS.md is a mandatory rule file that agents follow, this false claim will cause agents to write code importing fromMarkdown from mdast-util-from-markdown expecting it to resolve, leading to build/import failures.
Suggested change
| **Concrete tools available.** Both are already in `dependencies`: | |
| - TypeScript / JavaScript: `import ts from 'typescript'`; `ts.createSourceFile(name, content, ts.ScriptTarget.Latest, false, ts.ScriptKind.TS)` then walk `sourceFile.statements`. | |
| - Markdown: `import { fromMarkdown } from 'mdast-util-from-markdown'`; walk the resulting mdast tree (`inlineCode`, `code`, `heading`, `list`, etc.). | |
| **Concrete tools available.** TypeScript is already in `dependencies`; add `mdast-util-from-markdown` when a call site needs it: | |
| - TypeScript / JavaScript: `import ts from 'typescript'`; `ts.createSourceFile(name, content, ts.ScriptTarget.Latest, false, ts.ScriptKind.TS)` then walk `sourceFile.statements`. | |
| - Markdown: `import { fromMarkdown } from 'mdast-util-from-markdown'`; walk the resulting mdast tree (`inlineCode`, `code`, `heading`, `list`, etc.). Install with `npm install mdast-util-from-markdown` before first use. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
Ricky Eval ReviewRun: Passed: 5 | Needs human: 42 | Reviewable: 42 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesThese cases passed deterministic checks and include captured Ricky output for a human verdict against their REVIEW
|
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.
Summary
Codify the architectural rule that #86 and #88 both arrived at independently: when Ricky source code needs to inspect text whose grammar Ricky already understands (TS/JS, Markdown, JSON, shell), use a parser for that grammar — not substring match, not regex, not an LLM.
Why now
Two PRs landed in the same week with the same shape of bug:
auto-fix-loop.ts:hasRickyWorkflowAliasImportmarkdown-target-files.ts:extractTargetFilesFromMarkdownfrom 'node:fs'matched HEREDOC string-literal contentPATH_PATTERNmatched paths inside fenced code blocks + prose noisets.createSourceFile+ walkImportDeclarationnodesmdast-util-from-markdown+ walkinlineCodenodesBoth PRs also independently rejected LLM-based detection with the same reasoning (non-determinism, eval-suite breakage, hot-path latency, prompt-injection surface for paths that later drive shell commands). Worth capturing as a durable rule so the next person tempted to grep-and-substitute on TS source has the prior art.
What changed
AGENTS.md— new top-level# Ricky Source Code Conventionssection above the existing# Ricky Workflow Conventions. Holds one rule today (## Source-Text Analysis: Use Grammar-Aware Parsers, Not Regex) with reasoning, the rejected LLM alternative, available tooling (typescript,mdast-util-from-markdown), and a fallback ordering for when AST is genuinely too heavy.CLAUDE.md— auto-follows via the existing symlink, consistent with AGENTS.md:206 ("CLAUDE.mdshould remain a symlink toAGENTS.md. UpdateAGENTS.mdas the shared source of truth"). No separate Claude rule needed.Scope
Documentation-only. No code, dep, or test changes. The two referenced PRs (#86, #88) ship the actual code fixes; this PR captures the pattern those fixes share so we stop relearning it.
Test plan
readlink CLAUDE.md→AGENTS.md(symlink intact)head -180 CLAUDE.md | tail -10shows the new section through the symlink🤖 Generated with Claude Code