refactor: overhead-aware budgets, drop deps, limit tags#44
Merged
Conversation
📝 WalkthroughWalkthroughRemoved dependency rendering from SKILL.md generation and added an Changes
Sequence Diagram(s)sequenceDiagram
participant Sync as Sync Command
participant Gen as generateSkillMd
participant FS as FileSystem / BaseSkillData
participant Writer as writePromptFiles
participant Optimize as optimizeDocs / LLM
participant Sections as buildAllSectionPrompts
Sync->>Gen: request SKILL.md (no dependencies)
Gen-->>Sync: return SKILL.md
Sync->>FS: write SKILL.md and compute overheadLines = countLines(SKILL.md)
Sync->>Writer: write prompt files (include overheadLines)
Writer->>Optimize: optimizeDocs(overheadLines)
Optimize->>Sections: buildAllSectionPrompts(overheadLines)
Sections-->>Optimize: section prompts (budgets adjusted by overhead)
Optimize-->>Sync: optimized content / updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
harlan-zw
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
src/agent/clis/index.ts (1)
563-585:⚠️ Potential issue | 🟠 MajorBudget-aware prompts need budget-aware section cache keys.
overheadLinesnow changes the prompt content and section cap, but the reusablereadCachedSection()/writeSections()path is still keyed only bypackageName + version + outputFile. A section cached from a slimmer baseSKILL.mdwill be reused for a larger-overhead skill and skip regeneration, so the new line budgeting silently stops applying. The prompt-hash cache is safe here; the version cache needs the same budget signature or it should be bypassed for overhead-aware runs.Also applies to: 600-623, 725-733
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/clis/index.ts` around lines 563 - 585, The cached section lookup/playback must include the budget signature so overheadLines changes invalidate or bypass the version cache: update the cache key construction used by readCachedSection and writeSections (and any version-based cache path in optimizeDocs) to incorporate overheadLines (or a derived budget hash) and ensure buildAllSectionPrompts/section prompt generation uses the same signature; alternatively, when overheadLines is present and non-default, bypass the version cache path in optimizeDocs so sections are regenerated using the new line budget.src/commands/sync.ts (1)
1039-1054:⚠️ Potential issue | 🟡 MinorPortable prompt export still uses the fixed default overhead.
In
exportPortablePrompts(), prompt budgets are computed before the baseSKILL.mdexists, sobuildAllSectionPrompts()never receives the realoverheadLinesand falls back toDEFAULT_OVERHEAD. That makes the no-agent / portable path diverge from the sync flows updated in this PR. Precompute the base skill header/footer lines here too and pass them into the prompt builder.Also applies to: 1068-1089
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/sync.ts` around lines 1039 - 1054, The portable-export path computes prompt budgets before the base SKILL.md is created so buildAllSectionPrompts() receives DEFAULT_OVERHEAD instead of the real overheadLines; update exportPortablePrompts() to precompute the base skill header/footer line counts (the same way the sync flow does) and pass the resulting overheadLines into buildAllSectionPrompts() (and any other buildAllSectionPrompts() calls in this block, e.g., the calls around the 1068-1089 region) so prompt budgets match the non-portable sync flow rather than using DEFAULT_OVERHEAD.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/prompts/optional/budget.ts`:
- Around line 12-21: maxLines currently uses remainingLines() as a global cap
for each section, allowing the sum of per-section caps to exceed the actual body
allowance; update maxLines to compute a per-section share of the remaining
budget instead of using the full remaining for every section — e.g., calculate
remaining = remainingLines(overheadLines), derive a divisor from sectionCount
(use Math.max(1, sectionCount ?? 1)) or compute weights from
budgetScale(sectionCount) and the sum of scales, then clamp each section’s cap
to Math.min(Math.round(max * scale), Math.floor(remaining / divisor)) (or a
proportional share based on weights) so the combined caps cannot exceed
TOTAL_TARGET - overhead; retain references to remainingLines, maxLines,
budgetScale, TOTAL_TARGET and DEFAULT_OVERHEAD when making the change.
---
Outside diff comments:
In `@src/agent/clis/index.ts`:
- Around line 563-585: The cached section lookup/playback must include the
budget signature so overheadLines changes invalidate or bypass the version
cache: update the cache key construction used by readCachedSection and
writeSections (and any version-based cache path in optimizeDocs) to incorporate
overheadLines (or a derived budget hash) and ensure
buildAllSectionPrompts/section prompt generation uses the same signature;
alternatively, when overheadLines is present and non-default, bypass the version
cache path in optimizeDocs so sections are regenerated using the new line
budget.
In `@src/commands/sync.ts`:
- Around line 1039-1054: The portable-export path computes prompt budgets before
the base SKILL.md is created so buildAllSectionPrompts() receives
DEFAULT_OVERHEAD instead of the real overheadLines; update
exportPortablePrompts() to precompute the base skill header/footer line counts
(the same way the sync flow does) and pass the resulting overheadLines into
buildAllSectionPrompts() (and any other buildAllSectionPrompts() calls in this
block, e.g., the calls around the 1068-1089 region) so prompt budgets match the
non-portable sync flow rather than using DEFAULT_OVERHEAD.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d98aa5c2-0b44-46c6-ad7e-d63355f3cf62
📒 Files selected for processing (11)
src/agent/clis/index.tssrc/agent/clis/types.tssrc/agent/prompts/optional/api-changes.tssrc/agent/prompts/optional/best-practices.tssrc/agent/prompts/optional/budget.tssrc/agent/prompts/optional/custom.tssrc/agent/prompts/optional/types.tssrc/agent/prompts/prompt.tssrc/commands/sync-parallel.tssrc/commands/sync-shared.tssrc/commands/sync.ts
✅ Files skipped from review due to trivial changes (2)
- src/agent/clis/types.ts
- src/agent/prompts/optional/types.ts
harlan-zw
force-pushed
the
refactor/skill-md-tags
branch
from
153b18e to
0620c5d
Compare
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 (2)
src/commands/sync-parallel.ts (1)
337-356:⚠️ Potential issue | 🟠 MajorUse the resolved package name in phase 2.
pkgis still the raw CLI spec here. For inputs likevue@beta, prompt generation and theenhanceWithLLM(...)call below end up targetingvue@betainstead of the resolved package name that phase 1 used for the skill dir, references, and version state. Please switch these phase-2 calls todata.resolved.name(or the parsed package name) so dist-tag syncs keep pointing at the correct package.Also applies to: 367-370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/sync-parallel.ts` around lines 337 - 356, The loop over uncachedPkgs uses the raw CLI spec variable pkg when calling writePromptFiles and later enhanceWithLLM, causing phase-2 operations to target the un-resolved name (e.g., "vue@beta"); update these calls to use the resolved package identifier from skillData (data.resolved.name or parsed package name) instead of pkg, e.g., pass data.resolved.name for packageName and any other package-name arguments in writePromptFiles and enhanceWithLLM so prompts, versions, and dist-tag syncs reference the same resolved package used to build skillDir and version state.src/commands/sync.ts (1)
1084-1103:⚠️ Potential issue | 🟡 MinorPortable prompt export still skips the overhead-aware budget flow.
This path only generates the base
SKILL.mdat the end ofexportPortablePrompts(), afterbuildAllSectionPrompts(...)has already run, so portable prompts still fall back toDEFAULT_OVERHEAD.--agent none/ portable exports won't get the same section caps as the main sync flow unless this path also computes and passesoverheadLinesbefore prompt generation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/sync.ts` around lines 1084 - 1103, The portable export path in exportPortablePrompts() currently writes SKILL.md after buildAllSectionPrompts(...) but never computes or passes the overheadLines used by the main sync flow, so portable prompts fall back to DEFAULT_OVERHEAD; fix by computing overheadLines the same way the main sync flow does (replicating the overhead calculation used elsewhere), pass the computed overheadLines into buildAllSectionPrompts(...) when generating section prompts for portable exports, and include overheadLines in the options passed to generateSkillMd(...) so the exported SKILL.md and prompts respect the same overhead-aware caps (refer to exportPortablePrompts, buildAllSectionPrompts, generateSkillMd, and the SKILL.md write call to locate and update the code).
♻️ Duplicate comments (1)
src/agent/prompts/optional/budget.ts (1)
13-23:⚠️ Potential issue | 🟡 MinorDon’t force
minwhen the remaining body budget is already smaller than it.If
perSectiondrops belowmin,Math.max(min, ...)still restores the minimum cap, so the combined section limits can exceed the actual remaining body allowance again. LargeoverheadLinesvalues should reduce the cap belowmin, not reintroduce the overflow.Possible fix
/** Available body lines after overhead is subtracted */ function remainingLines(overheadLines?: number): number { - return TOTAL_TARGET - (overheadLines ?? DEFAULT_OVERHEAD) + return Math.max(0, TOTAL_TARGET - (overheadLines ?? DEFAULT_OVERHEAD)) } /** Scale max lines based on enabled section count and available remaining space. */ export function maxLines(min: number, max: number, sectionCount?: number, overheadLines?: number): number { const remaining = remainingLines(overheadLines) const sections = Math.max(1, sectionCount ?? 1) const perSection = Math.floor(remaining / sections) const scale = budgetScale(sectionCount) - return Math.max(min, Math.min(Math.round(max * scale), perSection)) + const scaledMax = Math.min(Math.round(max * scale), perSection) + return perSection < min ? scaledMax : Math.max(min, scaledMax) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/prompts/optional/budget.ts` around lines 13 - 23, The current maxLines enforces the min cap even when perSection (computed from remainingLines) is below min, allowing total sections to exceed the actual body budget; update maxLines so that if perSection < min it returns perSection (i.e., honor the remaining budget) otherwise apply the existing logic (compute scale via budgetScale, cap = Math.min(Math.round(max * scale), perSection) and then enforce min/max). Refer to remainingLines, maxLines, perSection, sectionCount and budgetScale to locate and implement this conditional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/commands/sync-parallel.ts`:
- Around line 337-356: The loop over uncachedPkgs uses the raw CLI spec variable
pkg when calling writePromptFiles and later enhanceWithLLM, causing phase-2
operations to target the un-resolved name (e.g., "vue@beta"); update these calls
to use the resolved package identifier from skillData (data.resolved.name or
parsed package name) instead of pkg, e.g., pass data.resolved.name for
packageName and any other package-name arguments in writePromptFiles and
enhanceWithLLM so prompts, versions, and dist-tag syncs reference the same
resolved package used to build skillDir and version state.
In `@src/commands/sync.ts`:
- Around line 1084-1103: The portable export path in exportPortablePrompts()
currently writes SKILL.md after buildAllSectionPrompts(...) but never computes
or passes the overheadLines used by the main sync flow, so portable prompts fall
back to DEFAULT_OVERHEAD; fix by computing overheadLines the same way the main
sync flow does (replicating the overhead calculation used elsewhere), pass the
computed overheadLines into buildAllSectionPrompts(...) when generating section
prompts for portable exports, and include overheadLines in the options passed to
generateSkillMd(...) so the exported SKILL.md and prompts respect the same
overhead-aware caps (refer to exportPortablePrompts, buildAllSectionPrompts,
generateSkillMd, and the SKILL.md write call to locate and update the code).
---
Duplicate comments:
In `@src/agent/prompts/optional/budget.ts`:
- Around line 13-23: The current maxLines enforces the min cap even when
perSection (computed from remainingLines) is below min, allowing total sections
to exceed the actual body budget; update maxLines so that if perSection < min it
returns perSection (i.e., honor the remaining budget) otherwise apply the
existing logic (compute scale via budgetScale, cap = Math.min(Math.round(max *
scale), perSection) and then enforce min/max). Refer to remainingLines,
maxLines, perSection, sectionCount and budgetScale to locate and implement this
conditional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0db1ceca-4e89-4ec2-b91d-b88ad5b11988
📒 Files selected for processing (13)
src/agent/clis/index.tssrc/agent/clis/types.tssrc/agent/prompts/optional/api-changes.tssrc/agent/prompts/optional/best-practices.tssrc/agent/prompts/optional/budget.tssrc/agent/prompts/optional/custom.tssrc/agent/prompts/optional/types.tssrc/agent/prompts/prompt.tssrc/agent/prompts/skill.tssrc/commands/install.tssrc/commands/sync-parallel.tssrc/commands/sync-shared.tssrc/commands/sync.ts
💤 Files with no reviewable changes (1)
- src/commands/install.ts
✅ Files skipped from review due to trivial changes (1)
- src/agent/prompts/optional/types.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/agent/prompts/optional/custom.ts
- src/agent/clis/types.ts
- src/agent/clis/index.ts
- src/agent/prompts/optional/api-changes.ts
- src/agent/prompts/optional/best-practices.ts
Remove the Deps line from generated SKILL.md entirely as it duplicates what's already in node_modules. Limit Tags to the 3 most recently released dist-tags, sorted by release date, with dates shown.
Section budgets now account for actual SKILL.md overhead (frontmatter, header, search block, footer). The base SKILL.md line count is passed through to budget functions so sections fill available space rather than using fixed estimates. Total target raised from 300 to 500 lines. Per-section limits bumped proportionally: best-practices 100/250 lines 6/15 items, api-changes 60/130 lines 8/18 items.
harlan-zw
force-pushed
the
refactor/skill-md-tags
branch
from
0620c5d to
4e4826b
Compare
6 tasks
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.
❓ Type of change
📚 Description
Cleans up generated SKILL.md content and makes section budgets adaptive.
SKILL.md cleanup:
**Deps:**line entirely (duplicatesnode_modules/<pkg>/package.json)**Tags:**to the 3 most recently released dist-tags, sorted byreleasedAtOverhead-aware budgets:
Summary by CodeRabbit