Skip to content

feat(aisimulate): guide efficient development with agent skills - #322

Merged
jthomson04 merged 4 commits into
mainfrom
jthomson04/aisimulate-performance-skills
Sep 29, 2026
Merged

jthomson04 merged 4 commits into
mainfrom
jthomson04/aisimulate-performance-skills

Conversation

@jthomson04

@jthomson04 jthomson04 commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

AISimulate feature and correctness changes can add substantial avoidable host cost. These two skills make efficient design and execution part of completion: resolve that overhead in the same PR before marking the implementation ready for merge.

  • Organize each guide around design, implementation, and validation/review. FPE covers numerical prediction and data lookup; replay covers scheduling, state transitions, and event execution. Consider preparation, repeated work, retained memory, caller costs, and cleanup.
  • Preserve required behavior and prefer simple designs. Distinguish necessary feature cost from implementation waste. Passing tests or documenting a slowdown does not justify avoidable overhead. Permit supported bounds for cold or small paths, with no universal slowdown limit.
  • Require measurements that exercise the changed paths and native replay when integrated cost can change. Preserve source-only review permissions and separate suspected risks, measured regressions, and inconclusive evidence.
  • Keep perf(perfmodel): index PreparedGrid lookups #239, perf(timing): reduce native replay evidence overhead #295, and perf(sglang): reduce admission checkpoint copies #321 as short examples of general failure patterns. Their solutions are not prescribed.
  • Share the skills from root .agents/skills/ through individual relative .claude/skills/ symlinks, with implicit invocation and subsystem ownership. Update descriptions and default prompts for ordinary development and review.

Replaces #91 with a source branch in ai-dynamo/aisimulate. No production code, benchmark framework, or CI policy changes.

Validation

  • Both quick_validate.py checks pass. Metadata, implicit invocation, relative links, symlink targets, and git diff --check pass.
  • Strict ownership validation covers all 3,385 tracked files; regenerated CODEOWNERS matches the committed file.
  • Codex discovers both enabled skills exactly once from the root, Python, and Rust directories. Claude Code startup discovers both from the root and Python directory; this checks discovery, not Claude model behavior.
  • Three independent read-only agents checked eight synthetic scenarios beyond the cited PRs. They required correction of repeated parsing/preparation and whole-state work, rejected an unjustified cache and deferring a measured 60% avoidable slowdown, and identified a benchmark that missed the change. They accepted a bounded cold path without a benchmark and accepted efficient FPE/replay implementations with sufficient evidence. Correctness and source-only limits were preserved.
  • No production benchmarks were run for this instruction change. The prior broader policy-test result remains separate: 106 passed and one unchanged test_full_ci_trusted_copy_verification failure because its subprocess used live gh instead of the fixture; that test passed when invoked directly. This suite was not rerun for the four guide/metadata edits.

@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⛔ Files ignored due to path filters (4)
  • .agents/skills/aisimulate-fpe-performance/SKILL.md is excluded by none and included by none
  • .agents/skills/aisimulate-fpe-performance/agents/openai.yaml is excluded by none and included by none
  • .agents/skills/aisimulate-replay-performance/SKILL.md is excluded by none and included by none
  • .agents/skills/aisimulate-replay-performance/agents/openai.yaml is excluded by none and included by none
⚙️ Run configuration

Configuration used: Repository: ai-dynamo/aisimulate/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: f5eb6887-539e-4575-aa24-b3e2cf6166a1

📥 Commits

Reviewing files that changed from the base of the PR and between 09969b1 and a6c0f04.

⛔ Files ignored due to path filters (4)
  • .agents/skills/aisimulate-fpe-performance/SKILL.md is excluded by none and included by none
  • .agents/skills/aisimulate-fpe-performance/agents/openai.yaml is excluded by none and included by none
  • .agents/skills/aisimulate-replay-performance/SKILL.md is excluded by none and included by none
  • .agents/skills/aisimulate-replay-performance/agents/openai.yaml is excluded by none and included by none

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: ai-dynamo/aisimulate/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 98b0b098-607e-418a-905b-3b8de7a1c624

📥 Commits

Reviewing files that changed from the base of the PR and between 409f47a and bb05532.

⛔ Files ignored due to path filters (4)
  • .agents/skills/aisimulate-fpe-performance/SKILL.md is excluded by none and included by none
  • .agents/skills/aisimulate-fpe-performance/agents/openai.yaml is excluded by none and included by none
  • .agents/skills/aisimulate-replay-performance/SKILL.md is excluded by none and included by none
  • .agents/skills/aisimulate-replay-performance/agents/openai.yaml is excluded by none and included by none
📒 Files selected for processing (4)
  • .claude/skills/aisimulate-fpe-performance
  • .claude/skills/aisimulate-replay-performance
  • .github/codeowners/areas.yaml
  • CODEOWNERS
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (22)
  • GitHub Check: Prediction Regression / Collect prediction snapshot (old)
  • GitHub Check: Prediction Regression / Collect prediction snapshot (new)
  • GitHub Check: Platform Wheels / Build wheels (manylinux_2_28_x86_64)
  • GitHub Check: Platform Wheels / Build wheels (macosx_arm64)
  • GitHub Check: Platform Wheels / Build wheels (manylinux_2_28_aarch64)
  • GitHub Check: Collector Data / Perf data sanity (informational)
  • GitHub Check: Collector Data / Check collector data
  • GitHub Check: Python 3.11 compatibility
  • GitHub Check: Public API Rust (amd64)
  • GitHub Check: Release Artifact Contract (amd64)
  • GitHub Check: Release Artifact Contract (arm64)
  • GitHub Check: Public API Rust (arm64)
  • GitHub Check: Application Test Wheel (amd64)
  • GitHub Check: Python Dependency Licenses
  • GitHub Check: Application Test Wheel (arm64)
  • GitHub Check: Cargo Deny (workspace, arm64)
  • GitHub Check: Engine Golden Regression
  • GitHub Check: Cargo Deny (workspace, amd64)
  • GitHub Check: Rust (arm64)
  • GitHub Check: Python 3.13 compatibility
  • GitHub Check: Rust (amd64)
  • GitHub Check: Rust feature modes
🧰 Additional context used
📓 Path-based instructions (3)
Read REVIEW.md before commenting.

⚙️ CodeRabbit configuration file

Files:

  • CODEOWNERS
Before making any change under: `python/aisimulate/src/aisimulate/generator/**` MUST read: `python/aisimulate/.claude/rules/generator-development.md` Before making any change under `python/aisimulate/collector/**` MUST read: `python/aisimul...

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • CODEOWNERS
Source excerpt: Only workflows under the repository-root `.github/workflows/` run for this repository.

📄 CodeRabbit inference engine (REVIEW.md)

Files:

  • CODEOWNERS
🔀 Multi-repo context ai-dynamo/dynamo, ai-dynamo/aiconfigurator

Linked repositories findings

ai-dynamo/dynamo

  • .agents/skills/ is canonical; skills/ and .claude/skills/ are compatibility symlinks. New shared skills should remain under .agents/skills/. [::ai-dynamo/dynamo::docs/fern/pages/agent-skills/overview.mdx]
  • The NVSkills workflow watches .agents/skills/**, so these additions correctly fall within the repository’s skill-validation CI scope. [::ai-dynamo/dynamo::.github/workflows/nvskills-team-request.yml]
  • Pre-commit validates skill frontmatter and the AGENTS.md index for .agents/skills/**; the new skill metadata/index entries are therefore the relevant local compatibility surface. [::ai-dynamo/dynamo::.pre-commit-config.yaml:195-198]

ai-dynamo/aiconfigurator

  • No direct references to the new FPE or replay skill names were found. Aiconfigurator’s skill rules apply only to its own .claude/skills/**/SKILL.md files and do not establish a cross-repository runtime contract. [::ai-dynamo/aiconfigurator::.coderabbit.yaml:27,56]
🔇 Additional comments (4)
.claude/skills/aisimulate-fpe-performance (1)

1-1: LGTM!

.claude/skills/aisimulate-replay-performance (1)

1-1: LGTM!

.github/codeowners/areas.yaml (1)

105-112: LGTM!

CODEOWNERS (1)

198-198: LGTM!

Also applies to: 202-202, 205-205, 209-209


📝 Summary

A detailed high-level summary could not be generated for this review. Here is an overview derived from the analyzed file changes:

  • .claude/skills/aisimulate-fpe-performance: ## AI-generated summary of changes
  • .claude/skills/aisimulate-replay-performance: ## AI-generated summary of changes
  • .github/codeowners/areas.yaml: ## AI-generated summary of changes
  • CODEOWNERS: ## AI-generated summary of changes

Walkthrough

The change adds Claude skill links to the corresponding agent skill directories. It also adds ownership rules for the Forward Pass Engine and Replay performance skill paths in shared ownership configuration and CODEOWNERS.

Changes

Performance skill paths and ownership

Layer / File(s) Summary
Skill path aliases and ownership rules
.claude/skills/*, .github/codeowners/areas.yaml, CODEOWNERS
Adds Claude symlinks for the FPE and replay skills. Assigns ownership of both .agents/ and .claude/ skill paths to the specified teams and maintainers.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Merge Risk: ⚪ Minimal · up to bb055

No actionable merge risk is established for these skill, symlink, and ownership changes.

🚥 Pre-merge checks | ✅ 7 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the purpose, scope, validation results, and known test limitation. It does not follow the required template and omits the Review map, Modeling or data provenance, and Tracking… Restructure the description using the repository template. Add the required headings and fields for risk level, reviewer starting points, public or serialized contract changes, compatibility or rollback concerns, exact test commands and res…
✅ Passed checks (7 passed)
Check name Status Explanation
Title check ✅ Passed The title identifies the addition of agent skills for more efficient AISimulate development. It is related to the changes, but it does not state the specific behavioral change as precisely as required…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cross-Layer Contract ✅ Passed PASS — The reviewed range changes only agent skill documentation/metadata, Claude symlinks, and CODEOWNERS data. It changes no Rust, Python, CLI, serialization, schema, or product test contract. Each …
Modeling And Data Evidence ✅ Passed The PR changes only skill documentation, agent metadata, symlinks, and CODEOWNERS. It does not add or refresh formulas, selection logic, model data, performance datasets, predicted outputs, golden fil…
Compatibility Boundaries ✅ Passed The PR changes only two agent skills, two relative Claude symlinks, and ownership metadata. No Python or Rust source, bindings, schemas, version files, dependency manifests, or artifact-contract files…
Review Evidence ✅ Passed The description names validation commands and results, including both quick_validate.py checks, git diff --check, ownership validation, and test_full_ci_trusted_copy_verification. It includes ne…
Full details: Description check

Explanation

The description explains the purpose, scope, validation results, and known test limitation. It does not follow the required template and omits the Review map, Modeling or data provenance, and Tracking sections, plus several required Evidence fields.

Resolution

Restructure the description using the repository template. Add the required headings and fields for risk level, reviewer starting points, public or serialized contract changes, compatibility or rollback concerns, exact test commands and results, commit identifiers, negative or boundary cases, modeling or data provenance, and issue or PR tracking. Write N/A where a section does not apply.


Comment @coderabbitai help to get the list of available commands.

@jthomson04 jthomson04 changed the title feat(aisimulate): require performance evidence in agent skills feat(aisimulate): guide efficient development with agent skills Sep 23, 2026

@Harrilee Harrilee 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.

Conditionally approve at commit 6ad4985. No blocking implementation issue; please address the small timing-scope correction below in both guides.

wall_time_ms encloses Replayer::run_inner (replayer.rs:368–547), including runtime setup and collector finalization. It excludes timing-model construction before replay (resolve_role_timing, python.rs:1751), and final timing-evidence/power aggregation after replay (python.rs:1924–1944). The current wording can make those preparation/reporting regressions appear covered by this metric. Name these exclusions and require an enclosing native-call or full CLI measurement when the changed work falls outside the timer.

The broader changed-path and caller-cost requirements are sound, so this is a localized documentation condition. The two skills preserve correctness, source-only review boundaries, and proportional evidence requirements.

Independent checks passed: both skill validators, metadata/implicit invocation, local links and Claude symlink resolution, strict ownership (3,385 tracked files), byte-identical regenerated CODEOWNERS/CONTRIBUTORS, and diff checks. Full CI run 35916729497 succeeded at this exact SHA. This is approval of the reviewed content; the PR remains draft.

Comment thread .agents/skills/aisimulate-replay-performance/SKILL.md Outdated
@jthomson04
jthomson04 marked this pull request as ready for review September 28, 2026 16:27
@jthomson04
jthomson04 requested review from a team as code owners September 28, 2026 16:27
@jthomson04
jthomson04 enabled auto-merge (squash) September 28, 2026 16:44
@jthomson04
jthomson04 force-pushed the jthomson04/aisimulate-performance-skills branch from bb05532 to 09969b1 Compare September 28, 2026 19:09
@copy-pr-bot

copy-pr-bot Bot commented Sep 28, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@jthomson04

Copy link
Copy Markdown
Contributor Author

/ok to test 09969b1

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
@jthomson04
jthomson04 force-pushed the jthomson04/aisimulate-performance-skills branch from 09969b1 to a6c0f04 Compare September 29, 2026 17:58
@jthomson04

Copy link
Copy Markdown
Contributor Author

/ok to test a6c0f04

@jthomson04
jthomson04 merged commit 011acbc into main Sep 29, 2026
68 checks passed
@jthomson04
jthomson04 deleted the jthomson04/aisimulate-performance-skills branch September 29, 2026 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants