[codex] polish repo knowledge skill#1
Open
kmshdev wants to merge 16 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds AST change-analysis and knowledge-diff helper modules, a structural adapter-shape checker with tests, a repo-level TDD fixture, and supporting docs, benchmark, and guidance updates. ChangesAST Change and Hinting Pipeline
Knowledge Diff and Baseline Script Modularization
Adapter Drift Detection, Fixture, and Guidance Updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
kmshdev
changed the title
There was a problem hiding this comment.
Pull request overview
This PR tightens repository knowledge-store validation by detecting “adapter drift” (tool-specific instruction files that point at AGENTS.md but also contain extra instructions) and adds a repo-level fixture script to forward-test the skill against a disposable agentskills/agentskills clone.
Changes:
- Add adapter-shape classification to
scripts/check_knowledge_store.py, including a newadapter-extra-contentwarning. - Add
unittestcoverage for valid/invalid adapter shapes and basic repo structure checks. - Document the structural adapter boundary in references/specs and add a repo-level TDD fixture script + quick reference entry.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_repo_level_tdd_fixture.py | Verifies fixture script constants (repo URL and default tmp work root). |
| tests/test_check_knowledge_store.py | Adds adapter-shape and repo-structure tests for check_knowledge_store.validate(). |
| scripts/repo_level_tdd_fixture.py | New script to clone/update a disposable agentskills checkout and run baseline/diff/health scripts. |
| scripts/check_knowledge_store.py | Adds adapter-shape detection and emits adapter-extra-content warnings. |
| SKILL.md | Updates quick reference + clarifies adapter expectations. |
| references/intent-layer-capture.md | Documents that adapter checks are structural (not semantic equivalence). |
| docs/superpowers/plans/2026-06-19-adapter-drift-detection.md | Implementation plan for adapter drift detection. |
| docs/specs/2026-06-19-adapter-drift-detection.md | Spec describing goals, non-goals, thresholds, and acceptance criteria. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| r"\b(auto[- ]?generated|generated|do not edit|source)\b", | ||
| re.IGNORECASE, | ||
| ) | ||
| ADAPTER_DECORATIVE_LINE_PATTERN = re.compile(r"^(#|<!--|-->|//)") |
Comment on lines
+59
to
+63
| if ( | ||
| len(lines) <= ADAPTER_SHORT_POINTER_MAX_LINES | ||
| and len(stripped) <= ADAPTER_SHORT_POINTER_MAX_CHARS | ||
| and all(ADAPTER_POINTER_PATTERN.search(line) for line in content_lines) | ||
| ): |
Comment on lines
+41
to
+42
| if refresh and repo.exists(): | ||
| shutil.rmtree(repo) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
tests/test_check_knowledge_store.py (1)
79-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the header-based false negatives.
The suite only checks paragraph-body extra content. It would miss a manual adapter whose header says
source of truth, and one whose extra instruction lives entirely in a markdown heading/comment line, even though both paths can currently suppressadapter-extra-content. Locking those cases in here will keep the classifier from regressing silently.🤖 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 `@tests/test_check_knowledge_store.py` around lines 79 - 165, Add regression tests in test_check_knowledge_store for the header-based false negatives by extending the existing adapter coverage around validate() and adapter_checks(). Include one CLAUDE.md fixture where the header itself claims a source of truth instead of pointing to AGENTS.md, and another where the extra instruction appears only in a markdown heading or comment line, then assert adapter-drift or adapter-extra-content is still reported as appropriate. Use the existing test patterns in test_generated_adapter_with_source_marker_passes and test_pointer_with_extra_instruction_content_warns to keep the cases aligned with the current classifier behavior.docs/specs/2026-06-19-adapter-drift-detection.md (1)
31-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVary the "Do not" repetition for readability.
Three successive non-goal bullets start with "Do not". Consider rephrasing for smoother flow, e.g., "Avoid semantic equivalence checking", "Skip LLM review...", "Keep
knowledge_diff.pyschema unchanged".🤖 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 `@docs/specs/2026-06-19-adapter-drift-detection.md` around lines 31 - 37, The Non-Goals list is too repetitive because several consecutive bullets start with “Do not,” so revise the wording for smoother readability. Update the bullets in the adapter drift detection spec to vary the phrasing while preserving meaning, using clearer alternatives such as “Avoid semantic adapter equivalence checking,” “Skip LLM review, embeddings, AST analysis, or runtime behavior checks,” and “Keep knowledge_diff.py JSON schema or path/status routing unchanged.”tests/test_repo_level_tdd_fixture.py (1)
20-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd behavior coverage for the fixture’s failure paths.
This only locks down the exported constants. The new logic that parses child stdout and chains baseline → diff is untested, so regressions there will still merge unnoticed.
🤖 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 `@tests/test_repo_level_tdd_fixture.py` around lines 20 - 28, The current test only verifies exported constants, so add coverage in RepoLevelTddFixtureTests for the fixture’s failure paths around the new child stdout parsing and baseline-to-diff chaining behavior. Extend the tests that load the module so they exercise the relevant fixture/helper methods that consume child process output and build the baseline/diff sequence, and assert the expected failures or error handling when stdout is malformed or chaining breaks. Keep the focus on the new logic rather than just AGENTSKILLS_REPO_URL and DEFAULT_WORK_ROOT.
🤖 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 `@docs/specs/2026-06-19-adapter-drift-detection.md`:
- Around line 124-136: The verification commands in the spec use hardcoded
absolute paths that make the document non-portable. Update the verification
section to use relative instructions from the repository root or
parameterized/environment-based paths instead of the author-specific
`/Users/kmsh/...` locations. Adjust the text around the verification commands so
it still references the same commands, but no longer depends on a local machine
path.
In `@docs/superpowers/plans/2026-06-19-adapter-drift-detection.md`:
- Around line 456-464: The validation command in this plan uses an
author-specific absolute path, which breaks portability. Update the referenced
validation step to use a parameterized or environment-agnostic way to locate the
quick_validate.py script, and make sure the instructions in this section clearly
tell readers how to find the script in their current setup instead of relying on
the local Codex install path.
- Around line 217-252: The Step 3 adapter_shape() plan is out of sync with the
actual scripts/check_knowledge_store.py implementation because it omits
decorative-line filtering and the stricter pointer validation. Update the
documented constants and adapter_shape() flow so it matches the real contract:
include ADAPTER_DECORATIVE_LINE_PATTERN, filter content_lines before
classification, and use the same all() pointer-pattern check and
generated/short-pointer ordering as the implementation. Keep the plan text
aligned with the existing adapter_shape(), ADAPTER_POINTER_PATTERN, and rel()
symbols so it mirrors the committed behavior exactly.
- Around line 398-409: The Step 3 command in the plan is using `uv run --script`
against `scripts/check_knowledge_store.py`, but that script does not have the
required PEP 723 metadata. Update the command to use the existing
`check_knowledge_store.py` entrypoint via `uv run python ... --json`, or add the
inline script metadata to `check_knowledge_store.py` if you want to keep
`--script`; keep the change aligned with the `Run markdown link and skill health
checks` step text.
In `@scripts/check_knowledge_store.py`:
- Around line 20-24: `adapter_shape()` in `scripts/check_knowledge_store.py` is
over-matching generated content and stripping too many lines, so manual adapters
can be misclassified as generated/short-pointer. Narrow
`ADAPTER_GENERATED_MARKER_PATTERN` so it only matches explicit boilerplate
markers and does not treat generic text like “source” as generated, and tighten
`ADAPTER_DECORATIVE_LINE_PATTERN` so `#`/comment-prefixed lines are only ignored
when they are true decorative headers, not meaningful prose before a pointer
line. Update the `adapter_shape()` checks to preserve real content and still
flag extra adapter text correctly.
In `@scripts/repo_level_tdd_fixture.py`:
- Around line 58-71: run_skill_script() should not assume any non-empty stdout
is valid JSON, because that can crash the fixture when a failed child prints
plain text. Update the result handling in run_skill_script so json.loads() is
only used when stdout is actually JSON, and otherwise preserve the raw stdout as
the captured failure payload; keep the returned structure with command,
returncode, stdout, and stderr unchanged.
- Around line 80-89: The fixture currently always writes baseline.json and runs
knowledge_diff.py even when find_agents_baseline.py fails, which can create a
misleading follow-up failure. Update the summarize() flow in
repo_level_tdd_fixture.py so the diff step is only invoked after a successful
baseline run, using the existing run_skill_script result from baseline_script to
decide whether to continue; if the baseline fails, stop before writing the
baseline file or calling diff_script. Keep the logic centered around
summarize(), baseline_script, diff_script, and baseline_path so it’s easy to
locate.
---
Nitpick comments:
In `@docs/specs/2026-06-19-adapter-drift-detection.md`:
- Around line 31-37: The Non-Goals list is too repetitive because several
consecutive bullets start with “Do not,” so revise the wording for smoother
readability. Update the bullets in the adapter drift detection spec to vary the
phrasing while preserving meaning, using clearer alternatives such as “Avoid
semantic adapter equivalence checking,” “Skip LLM review, embeddings, AST
analysis, or runtime behavior checks,” and “Keep knowledge_diff.py JSON schema
or path/status routing unchanged.”
In `@tests/test_check_knowledge_store.py`:
- Around line 79-165: Add regression tests in test_check_knowledge_store for the
header-based false negatives by extending the existing adapter coverage around
validate() and adapter_checks(). Include one CLAUDE.md fixture where the header
itself claims a source of truth instead of pointing to AGENTS.md, and another
where the extra instruction appears only in a markdown heading or comment line,
then assert adapter-drift or adapter-extra-content is still reported as
appropriate. Use the existing test patterns in
test_generated_adapter_with_source_marker_passes and
test_pointer_with_extra_instruction_content_warns to keep the cases aligned with
the current classifier behavior.
In `@tests/test_repo_level_tdd_fixture.py`:
- Around line 20-28: The current test only verifies exported constants, so add
coverage in RepoLevelTddFixtureTests for the fixture’s failure paths around the
new child stdout parsing and baseline-to-diff chaining behavior. Extend the
tests that load the module so they exercise the relevant fixture/helper methods
that consume child process output and build the baseline/diff sequence, and
assert the expected failures or error handling when stdout is malformed or
chaining breaks. Keep the focus on the new logic rather than just
AGENTSKILLS_REPO_URL and DEFAULT_WORK_ROOT.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17d22ef6-93ca-4e03-a4ac-fc832e937266
📒 Files selected for processing (8)
SKILL.mddocs/specs/2026-06-19-adapter-drift-detection.mddocs/superpowers/plans/2026-06-19-adapter-drift-detection.mdreferences/intent-layer-capture.mdscripts/check_knowledge_store.pyscripts/repo_level_tdd_fixture.pytests/test_check_knowledge_store.pytests/test_repo_level_tdd_fixture.py
Comment on lines
+20
to
+24
| ADAPTER_GENERATED_MARKER_PATTERN = re.compile( | ||
| r"\b(auto[- ]?generated|generated|do not edit|source)\b", | ||
| re.IGNORECASE, | ||
| ) | ||
| ADAPTER_DECORATIVE_LINE_PATTERN = re.compile(r"^(#|<!--|-->|//)") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The new heuristics can hide real adapter drift.
adapter_shape() currently treats any header containing the word source as generated, and it drops every #... / comment-prefixed line before the short-pointer check. That means a manual adapter like Canonical source is AGENTS.md or # Run npm test before docs updates plus a pointer line will be skipped as generated-pointer / short-pointer instead of warning on extra content. Please narrow the generated-marker match and only ignore truly decorative boilerplate, not arbitrary heading/comment text.
Also applies to: 56-63, 157-158
🤖 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 `@scripts/check_knowledge_store.py` around lines 20 - 24, `adapter_shape()` in
`scripts/check_knowledge_store.py` is over-matching generated content and
stripping too many lines, so manual adapters can be misclassified as
generated/short-pointer. Narrow `ADAPTER_GENERATED_MARKER_PATTERN` so it only
matches explicit boilerplate markers and does not treat generic text like
“source” as generated, and tighten `ADAPTER_DECORATIVE_LINE_PATTERN` so
`#`/comment-prefixed lines are only ignored when they are true decorative
headers, not meaningful prose before a pointer line. Update the
`adapter_shape()` checks to preserve real content and still flag extra adapter
text correctly.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@scripts/ast_change_config.py`:
- Line 89: The Haskell AST profile in ast_change_config.py uses a misspelled
node kind, so it will not match type synonym declarations. Update the Haskell
entry in the config to use the tree-sitter-haskell symbol type_alias instead of
type_synomym, keeping the existing function and data_type mappings unchanged.
In `@scripts/knowledge_diff_git.py`:
- Around line 41-60: The changed_files() function currently only aggregates
committed, working-tree, and staged paths via run_git diff output, so brand-new
untracked files are missed. Update changed_files() in
scripts/knowledge_diff_git.py to also collect untracked paths (for example by
querying git status for untracked entries) and merge them into the seen map with
an appropriate status before returning the sorted list. Keep the existing
normalize_status() handling and make sure the new untracked-path source uses the
same scope filtering logic as the current git diff calls.
- Around line 24-29: `load_baseline()` only checks for the presence of
`agent_files`, so non-array values slip through and are misclassified as a
missing baseline. Update the validation in `load_baseline()` to verify that
`data["agent_files"]` is actually a list/array before returning, and raise a
`ValueError` for any malformed payload so invalid baseline files fail fast
instead of being treated as “no baseline.”
In `@scripts/knowledge_diff_summary.py`:
- Around line 46-51: The ancestor-scope scan in the loop over agents is
incorrectly collecting files that should be owned by a deeper nested agent.
Update the logic around changed_files() and nearest_agent() so a path is only
added when the current scan’s agent is the nearest owning scope, or skip
ancestor results that fall under a deeper agent with a newer last_commit. Use
the existing symbols changed_files, nearest_agent, all_changes, and change_entry
to keep the summary from double-reporting nested files.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: eccd6ee4-78c9-4444-b9da-35ce8a96503a
📒 Files selected for processing (28)
.plugin-eval/benchmark.jsonagents/openai.yamldocs/evaluations/2026-06-30-agents-guidance-alignment.mddocs/superpowers/plans/2026-06-30-update-repo-knowledge-skill-polish.mdscripts/ast_change_analysis.pyscripts/ast_change_config.pyscripts/ast_change_hint_building.pyscripts/ast_change_hints.pyscripts/ast_change_io.pyscripts/ast_change_match.pyscripts/ast_change_paths.pyscripts/ast_change_summary.pyscripts/check_knowledge_adapter_emit.pyscripts/check_knowledge_adapter_shape.pyscripts/check_knowledge_adapters.pyscripts/check_knowledge_common.pyscripts/check_knowledge_docs.pyscripts/check_knowledge_links.pyscripts/check_knowledge_store.pyscripts/find_agents_baseline.pyscripts/find_agents_collect.pyscripts/find_agents_git.pyscripts/knowledge_diff.pyscripts/knowledge_diff_classify.pyscripts/knowledge_diff_config.pyscripts/knowledge_diff_git.pyscripts/knowledge_diff_summary.pytests/test_ast_change_hints.py
| "css": ["rule_set", "at_rule"], | ||
| "elixir": ["call"], | ||
| "go": ["function_declaration", "method_declaration", "type_declaration"], | ||
| "haskell": ["function", "type_synomym", "data_type"], |
|
|
||
| **Goal:** Improve the `update-repo-knowledge` skill metadata, static quality, benchmark scenarios, and evaluation coverage while preserving strict repository-knowledge checker semantics. | ||
|
|
||
| **Architecture:** Make changes in the source skill repo at `/Users/kmsh/Researcher/update-repo-knowledge`, validate there, then copy the validated files into the installed skill at `/Users/kmsh/.agents/skills/update-repo-knowledge`. Keep `SKILL.md` concise and put evaluation detail in docs so the skill keeps using progressive disclosure. Use disposable target repositories for behavior validation instead of weakening checker behavior for this skill package. |
Comment on lines
+22
to
+24
| - Local guide: `/Users/kmsh/.codex/.agent/agents-md-skills-md.md` | ||
| - Installed skill target: `/Users/kmsh/.agents/skills/update-repo-knowledge` | ||
| - Source skill repo: `/Users/kmsh/Researcher/update-repo-knowledge` |
Comment on lines
+37
to
+41
| lines = [line.strip() for line in stripped.splitlines() if line.strip()] | ||
| header = "\n".join(lines[:ADAPTER_GENERATED_HEADER_LINES]) | ||
| if ADAPTER_GENERATED_MARKER_PATTERN.search(header): | ||
| return "generated-pointer" | ||
| if is_short_pointer(stripped, lines): |
|
|
||
| def remove_disposable_clone(repo: Path) -> None: | ||
| """Remove an existing checkout only when it is clearly disposable.""" | ||
| if repo.is_symlink() or not repo.is_dir() or not (repo / ".git").exists(): |
There was a problem hiding this comment.
🧹 Nitpick comments (8)
docs/superpowers/plans/2026-06-30-update-repo-knowledge-skill-polish.md (8)
588-588: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove hardcoded plugin-eval path (repeat).
Same local evaluation path issue:
$HOME/.codex/plugins/cache/openai-curated-remote/plugin-eval/0.1.2/scripts/plugin-eval.jsshould be parameterized per PR objectives.🤖 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 `@docs/superpowers/plans/2026-06-30-update-repo-knowledge-skill-polish.md` at line 588, The evaluation command still hardcodes the local plugin-eval script path, which should be parameterized instead of baked into the plan. Update the command in the affected plan step to use the repo’s configurable plugin-eval location or a variable/placeholder aligned with the PR objectives, and keep the change localized to the plan entry that references plugin-eval.
691-692: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove hardcoded plugin-eval path (repeat).
Same local evaluation path issue at line 692. This is the third occurrence of the hardcoded plugin-eval path in the document.
🤖 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 `@docs/superpowers/plans/2026-06-30-update-repo-knowledge-skill-polish.md` around lines 691 - 692, The document still hardcodes the local plugin-eval script path in the evaluation command, which should be removed to avoid repeating environment-specific instructions. Update the command in the affected section to use a portable reference or placeholder for the plugin-eval invocation, and keep the guidance aligned with the rest of the plan by referring to the same command context instead of the absolute path.
28-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRemove or parameterize local sync target paths.
The file structure list repeatedly references
$HOME/.agents/skills/update-repo-knowledge/as a sync target (lines 33-36). These are local evaluation/installation paths that should not be hardcoded in repository documentation. Use a parameterized reference like<SKILL_HOME>or describe as "installed skill location" without hardcoding the path.🤖 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 `@docs/superpowers/plans/2026-06-30-update-repo-knowledge-skill-polish.md` around lines 28 - 36, The plan in this document hardcodes `$HOME/.agents/skills/update-repo-knowledge/` as the sync target, which should be parameterized instead. Update the affected bullet items to use a placeholder like `<SKILL_HOME>` or a generic “installed skill location” reference, and keep the source-file entries (`agents/openai.yaml`, `scripts/ast_change_hints.py`, `tests/test_ast_change_hints.py`, `.plugin-eval/benchmark.json`) unchanged while removing local path assumptions.
314-314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove hardcoded plugin-eval path.
The path
$HOME/.codex/plugins/cache/openai-curated-remote/plugin-eval/0.1.2/scripts/plugin-eval.jsis a local evaluation path that varies across installations and versions. Per the PR objective to remove local evaluation paths from docs, replace with a parameterized reference like<PLUGIN_EVAL_ROOT>/scripts/plugin-eval.jsor describe it as "the plugin-eval CLI in your Codex installation."🤖 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 `@docs/superpowers/plans/2026-06-30-update-repo-knowledge-skill-polish.md` at line 314, The docs entry still hardcodes the local plugin-eval path, which should be removed to avoid installation-specific references. Update the relevant example in the plan doc to use a parameterized placeholder like <PLUGIN_EVAL_ROOT>/scripts/plugin-eval.js or describe invoking the plugin-eval CLI from the user’s Codex installation, keeping the guidance generic and version-agnostic.
652-655: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
py_compile scripts/*.pymay fail on non-Python files or subdirectories.The glob
scripts/*.pyassumes all Python files are directly inscripts/with.pyextension. Ifscripts/contains subdirectories with Python modules or non-Python files without.pyextension, this command may miss files or be unnecessary. Considerpython -m compileall scripts/for recursive compilation, or ensure the glob matches your layout.🤖 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 `@docs/superpowers/plans/2026-06-30-update-repo-knowledge-skill-polish.md` around lines 652 - 655, The validation step using py_compile is too narrow and may miss Python modules outside the top-level scripts glob. Update the command sequence around the existing unittest and py_compile steps to use a layout-aware compilation approach, such as compileall on the scripts tree, or otherwise align the command with the actual scripts directory structure. Keep the change localized to the setup/verification instructions so the documented workflow matches the repo’s Python file layout.
698-708: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRemove or parameterize local sync target paths in Task 5.
The sync commands (lines 703-707) and validation directory (line 717) hardcode
$HOME/.agents/skills/update-repo-knowledge/. These are local evaluation/installation paths that should be parameterized or replaced with environment-agnostic instructions per the PR objective to remove local evaluation paths from docs.🤖 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 `@docs/superpowers/plans/2026-06-30-update-repo-knowledge-skill-polish.md` around lines 698 - 708, The Task 5 sync and validation steps still hardcode the local $HOME/.agents/skills/update-repo-knowledge path, which should be removed from the docs. Update the instructions in the affected plan section to use a parameterized or environment-agnostic destination instead of fixed local evaluation/installation paths, and make the validation directory reference follow the same pattern. Keep the guidance aligned with the existing Task 5 steps and their sync/validation commands so readers can apply the setup in any environment.
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRemove local evaluation paths from docs per PR objective.
Lines 22 and 23 hardcode local filesystem paths (
$HOME/.codex/.agent/agents-md-skills-md.md,$HOME/.agents/skills/update-repo-knowledge) that tie this documentation to a specific local environment. The PR objectives explicitly mention "remove local evaluation paths from docs" as a commit concern. Replace with environment-agnostic descriptions or parameterize with<SKILL_HOME>or similar placeholder.🤖 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 `@docs/superpowers/plans/2026-06-30-update-repo-knowledge-skill-polish.md` around lines 22 - 24, Remove the hardcoded local filesystem references in the plan doc and replace them with environment-agnostic placeholders or generic descriptions. Update the entries under the plan section so the skill locations are described via a neutral symbol like <SKILL_HOME> or equivalent, keeping the wording independent of any specific machine while preserving the intent of the local guide, installed skill target, and source skill repo references.
451-451: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove hardcoded plugin-eval path (repeat).
Same issue as line 314:
$HOME/.codex/plugins/cache/openai-curated-remote/plugin-eval/0.1.2/scripts/plugin-eval.jsis a local evaluation path that should be parameterized or described generically per PR objectives.🤖 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 `@docs/superpowers/plans/2026-06-30-update-repo-knowledge-skill-polish.md` at line 451, The command in the plan still hardcodes the local plugin-eval script path, which should be generalized. Update the instruction around the benchmark command to reference plugin-eval conceptually or via a configurable path/variable instead of the specific $HOME/.codex/plugins/cache/openai-curated-remote/plugin-eval/0.1.2/scripts/plugin-eval.js location, keeping the guidance aligned with the existing plan wording.
🤖 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.
Nitpick comments:
In `@docs/superpowers/plans/2026-06-30-update-repo-knowledge-skill-polish.md`:
- Line 588: The evaluation command still hardcodes the local plugin-eval script
path, which should be parameterized instead of baked into the plan. Update the
command in the affected plan step to use the repo’s configurable plugin-eval
location or a variable/placeholder aligned with the PR objectives, and keep the
change localized to the plan entry that references plugin-eval.
- Around line 691-692: The document still hardcodes the local plugin-eval script
path in the evaluation command, which should be removed to avoid repeating
environment-specific instructions. Update the command in the affected section to
use a portable reference or placeholder for the plugin-eval invocation, and keep
the guidance aligned with the rest of the plan by referring to the same command
context instead of the absolute path.
- Around line 28-36: The plan in this document hardcodes
`$HOME/.agents/skills/update-repo-knowledge/` as the sync target, which should
be parameterized instead. Update the affected bullet items to use a placeholder
like `<SKILL_HOME>` or a generic “installed skill location” reference, and keep
the source-file entries (`agents/openai.yaml`, `scripts/ast_change_hints.py`,
`tests/test_ast_change_hints.py`, `.plugin-eval/benchmark.json`) unchanged while
removing local path assumptions.
- Line 314: The docs entry still hardcodes the local plugin-eval path, which
should be removed to avoid installation-specific references. Update the relevant
example in the plan doc to use a parameterized placeholder like
<PLUGIN_EVAL_ROOT>/scripts/plugin-eval.js or describe invoking the plugin-eval
CLI from the user’s Codex installation, keeping the guidance generic and
version-agnostic.
- Around line 652-655: The validation step using py_compile is too narrow and
may miss Python modules outside the top-level scripts glob. Update the command
sequence around the existing unittest and py_compile steps to use a layout-aware
compilation approach, such as compileall on the scripts tree, or otherwise align
the command with the actual scripts directory structure. Keep the change
localized to the setup/verification instructions so the documented workflow
matches the repo’s Python file layout.
- Around line 698-708: The Task 5 sync and validation steps still hardcode the
local $HOME/.agents/skills/update-repo-knowledge path, which should be removed
from the docs. Update the instructions in the affected plan section to use a
parameterized or environment-agnostic destination instead of fixed local
evaluation/installation paths, and make the validation directory reference
follow the same pattern. Keep the guidance aligned with the existing Task 5
steps and their sync/validation commands so readers can apply the setup in any
environment.
- Around line 22-24: Remove the hardcoded local filesystem references in the
plan doc and replace them with environment-agnostic placeholders or generic
descriptions. Update the entries under the plan section so the skill locations
are described via a neutral symbol like <SKILL_HOME> or equivalent, keeping the
wording independent of any specific machine while preserving the intent of the
local guide, installed skill target, and source skill repo references.
- Line 451: The command in the plan still hardcodes the local plugin-eval script
path, which should be generalized. Update the instruction around the benchmark
command to reference plugin-eval conceptually or via a configurable
path/variable instead of the specific
$HOME/.codex/plugins/cache/openai-curated-remote/plugin-eval/0.1.2/scripts/plugin-eval.js
location, keeping the guidance aligned with the existing plan wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0b10280-121a-4fc7-9ec3-8c959125621f
📒 Files selected for processing (16)
docs/evaluations/2026-06-30-agents-guidance-alignment.mddocs/specs/2026-06-19-adapter-drift-detection.mddocs/superpowers/plans/2026-06-19-adapter-drift-detection.mddocs/superpowers/plans/2026-06-30-update-repo-knowledge-skill-polish.mdscripts/ast_change_config.pyscripts/check_knowledge_adapter_shape.pyscripts/check_knowledge_store.pyscripts/find_agents_baseline.pyscripts/knowledge_diff.pyscripts/knowledge_diff_git.pyscripts/knowledge_diff_summary.pyscripts/repo_level_tdd_fixture.pytests/test_ast_change_hints.pytests/test_check_knowledge_store.pytests/test_knowledge_diff.pytests/test_repo_level_tdd_fixture.py
💤 Files with no reviewable changes (1)
- scripts/check_knowledge_store.py
✅ Files skipped from review due to trivial changes (3)
- docs/evaluations/2026-06-30-agents-guidance-alignment.md
- docs/superpowers/plans/2026-06-19-adapter-drift-detection.md
- docs/specs/2026-06-19-adapter-drift-detection.md
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/test_check_knowledge_store.py
- scripts/check_knowledge_adapter_shape.py
- tests/test_repo_level_tdd_fixture.py
- scripts/ast_change_config.py
- scripts/knowledge_diff_git.py
- scripts/find_agents_baseline.py
- scripts/knowledge_diff_summary.py
- scripts/repo_level_tdd_fixture.py
Comment on lines
+58
to
+66
| def remove_disposable_clone(repo: Path) -> None: | ||
| """Remove an existing checkout only when it is clearly disposable.""" | ||
| git_dir = repo / ".git" | ||
| if repo.is_symlink() or not repo.is_dir() or not git_dir.is_dir(): | ||
| raise RuntimeError( | ||
| "Refusing to refresh non-disposable agentskills path: " | ||
| f"{repo}. Delete it manually or choose a different --work-root." | ||
| ) | ||
| shutil.rmtree(repo) |
Comment on lines
+110
to
+113
| baseline_json = json.dumps(baseline["stdout"]) | ||
| baseline_path = target_repo.parent / "baseline.json" | ||
| baseline_path.write_text(baseline_json, encoding="utf-8") | ||
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/check_knowledge_traversal.py`:
- Around line 44-53: The tracked_and_unignored_files() helper is including
cached entries even when the file has been deleted from the worktree, which lets
stale paths reach knowledge_files() and downstream checks. Update
tracked_and_unignored_files() to ignore cached paths that no longer exist on
disk before appending them, so check_agent_files() and check_docs_indexes() only
operate on files present in the current checkout.
- Around line 29-36: The run_git helper currently assumes git exists, so
subprocess.run can raise before knowledge_files() gets a chance to use
fallback_files(). Update run_git to gracefully handle missing git by catching
the executable-not-found failure and returning a completed-process-like result
with a nonzero status and empty output, then let knowledge_files() continue into
fallback_files() as intended.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15464ff5-16d9-4d70-a94e-01f24756b4df
📒 Files selected for processing (10)
scripts/ast_change_hints.pyscripts/ast_change_match.pyscripts/check_knowledge_adapters.pyscripts/check_knowledge_docs.pyscripts/check_knowledge_links.pyscripts/check_knowledge_traversal.pyscripts/repo_level_tdd_fixture.pytests/test_ast_change_hints.pytests/test_check_knowledge_store.pytests/test_repo_level_tdd_fixture.py
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/check_knowledge_links.py
- scripts/ast_change_match.py
- scripts/repo_level_tdd_fixture.py
Comment on lines
+29
to
+36
| def run_git(repo: Path, args: list[str]) -> subprocess.CompletedProcess[str]: | ||
| return subprocess.run( | ||
| ["git", "-C", str(repo), *args], | ||
| check=False, | ||
| text=True, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle missing git without crashing the fallback path.
If git is not installed, subprocess.run(...) raises before knowledge_files() can reach fallback_files(). That turns the non-worktree path into a hard failure instead of a graceful scan.
Proposed fix
def run_git(repo: Path, args: list[str]) -> subprocess.CompletedProcess[str]:
- return subprocess.run(
- ["git", "-C", str(repo), *args],
- check=False,
- text=True,
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE,
- )
+ command = ["git", "-C", str(repo), *args]
+ try:
+ return subprocess.run(
+ command,
+ check=False,
+ text=True,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE,
+ )
+ except OSError as error:
+ return subprocess.CompletedProcess(command, 127, stdout="", stderr=str(error))📝 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
| def run_git(repo: Path, args: list[str]) -> subprocess.CompletedProcess[str]: | |
| return subprocess.run( | |
| ["git", "-C", str(repo), *args], | |
| check=False, | |
| text=True, | |
| stdout=subprocess.PIPE, | |
| stderr=subprocess.PIPE, | |
| ) | |
| def run_git(repo: Path, args: list[str]) -> subprocess.CompletedProcess[str]: | |
| command = ["git", "-C", str(repo), *args] | |
| try: | |
| return subprocess.run( | |
| command, | |
| check=False, | |
| text=True, | |
| stdout=subprocess.PIPE, | |
| stderr=subprocess.PIPE, | |
| ) | |
| except OSError as error: | |
| return subprocess.CompletedProcess(command, 127, stdout="", stderr=str(error)) |
🧰 Tools
🪛 ast-grep (0.44.0)
[error] 29-35: Command coming from incoming request
Context: subprocess.run(
["git", "-C", str(repo), *args],
check=False,
text=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.20)
[error] 30-30: subprocess call: check for execution of untrusted input
(S603)
[error] 31-31: Starting a process with a partial executable path
(S607)
🤖 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 `@scripts/check_knowledge_traversal.py` around lines 29 - 36, The run_git
helper currently assumes git exists, so subprocess.run can raise before
knowledge_files() gets a chance to use fallback_files(). Update run_git to
gracefully handle missing git by catching the executable-not-found failure and
returning a completed-process-like result with a nonzero status and empty
output, then let knowledge_files() continue into fallback_files() as intended.
Comment on lines
+44
to
+53
| def tracked_and_unignored_files(repo: Path) -> list[Path]: | ||
| result = run_git(repo, ["ls-files", "--cached", "--others", "--exclude-standard"]) | ||
| if result.returncode != 0: | ||
| return [] | ||
| paths = [] | ||
| for line in result.stdout.splitlines(): | ||
| if not line.strip(): | ||
| continue | ||
| paths.append(repo / line) | ||
| return sorted(paths) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Filter out cached paths that no longer exist in the worktree.
git ls-files --cached --others --exclude-standard still returns tracked files after a local delete. That makes knowledge_files() feed stale paths into downstream checks, so check_agent_files() / check_docs_indexes() can infer deleted AGENTS.md or index.md files from the index instead of the current checkout.
Proposed fix
def tracked_and_unignored_files(repo: Path) -> list[Path]:
result = run_git(repo, ["ls-files", "--cached", "--others", "--exclude-standard"])
if result.returncode != 0:
return []
paths = []
for line in result.stdout.splitlines():
if not line.strip():
continue
- paths.append(repo / line)
+ path = repo / line
+ if path.exists() or path.is_symlink():
+ paths.append(path)
return sorted(paths)📝 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
| def tracked_and_unignored_files(repo: Path) -> list[Path]: | |
| result = run_git(repo, ["ls-files", "--cached", "--others", "--exclude-standard"]) | |
| if result.returncode != 0: | |
| return [] | |
| paths = [] | |
| for line in result.stdout.splitlines(): | |
| if not line.strip(): | |
| continue | |
| paths.append(repo / line) | |
| return sorted(paths) | |
| def tracked_and_unignored_files(repo: Path) -> list[Path]: | |
| result = run_git(repo, ["ls-files", "--cached", "--others", "--exclude-standard"]) | |
| if result.returncode != 0: | |
| return [] | |
| paths = [] | |
| for line in result.stdout.splitlines(): | |
| if not line.strip(): | |
| continue | |
| path = repo / line | |
| if path.exists() or path.is_symlink(): | |
| paths.append(path) | |
| return sorted(paths) |
🤖 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 `@scripts/check_knowledge_traversal.py` around lines 44 - 53, The
tracked_and_unignored_files() helper is including cached entries even when the
file has been deleted from the worktree, which lets stale paths reach
knowledge_files() and downstream checks. Update tracked_and_unignored_files() to
ignore cached paths that no longer exist on disk before appending them, so
check_agent_files() and check_docs_indexes() only operate on files present in
the current checkout.
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
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
Validation
python3 -m unittest discover -s tests -vpython3 -m py_compile scripts/*.pyuv run --script scripts/repo_level_tdd_fixture.py --refreshpython3 -m unittest discover -s tests -vand metadata/benchmark JSON checks100/100, gradeA, no fail/warn checksSummary by CodeRabbit
adapter-extra-contentwarning.update-repo-knowledgeagent to refresh knowledge using these checks.