Porting skill improvement - #75
Merged
Merged
Conversation
The skill wrote agents/, workflow/ and config/ beside an untouched source tree. It now duplicates the source into .car/app, edits adapters in place inside that copy, and keeps every declaration in .car/config. Two rules came out of running it end to end on a real LangGraph project: M27 -- root the copy at the source's *import* root, not its repository root. A project whose modules import each other as top-level names from under src/ builds green, reports healthy, and answers `No agent loaded` on the first request when the copy is rooted one level too high. validate.py already caught this as V031; nobody had run it before building. M25 -- two agents may not share one entrypoint, because each agent's stub is written over its own entrypoint and the second would land on the first. V023 is rewritten around the same idea: the workflow must reach an agent through the module the build replaces with a stub, so a flat name or a package re-export now reports where the stub actually lands. V020 becomes the shared entrypoint check, V032 requires config/ beside app/, and the stale `stub_two_destinations` capability is gone -- a stub has one destination now. Also applied Anthropic's skill-authoring guidance: the invented `compatibility` frontmatter key is folded into the body, the description carries the terms that should trigger it (`ventis`, `.car`, `ventis build`), the workflow opens with a copyable checklist, the validator is stated as a loop with an exit condition rather than a one-shot, references over 100 lines gained a table of contents, and a concrete worked example is added under references/.
50 of the corpus's 73 findings are on this skill, and none is a broken feature: every one is "the document did not say, so the porter worked it out." Six of the seven clusters are closed here. Stub-overwrites-entrypoint (12 findings / 9 repos). The build replaces an agent's entrypoint module wholesale in every other image, and the skill documented the fact without its consequences. SKILL.md now gates the entrypoint choice on the five properties that break: another image reading the module for real, a package __init__ re-exporting from it, a path segment that is not an identifier, relative imports in the entrypoint, and module-scope code that performs a real run. runtime-contract.md says what survives around a stub. The joke_writer example claimed the yaml basename names the stub; it does not, and it contradicted runtime-contract.md:73. requirements (7 findings / 7 repos). M10 said only "a list of strings". Added how to build the list from the import graph the image executes rather than the code the porter wrote, how to version it against the source's own lockfile and era, and a resolve-before-you-build step. validate.py false positives (7 findings / 6 repos). `import autogen` is shipped by three distributions and llama-index by a family, so both were permanent W006 noise; the table now maps an import to every distribution that provides it. The import check walked only the entrypoint's own AST, so a dependency one hop away was invisible -- it now walks the transitive graph inside the copy, package __init__ files included, and covers the workflow entry too. New V033/V034/V035 turn three container-startup crashes into preflight errors. sys.stdlib_module_names is 3.10+, so the stdlib filter was empty on older interpreters and the wider walk would have flagged every `import os`; derived it from the interpreter instead. llm_proxy env spellings (5 findings / 5 repos, 3 blocking). One spelling per provider was wrong: langchain_anthropic reads ANTHROPIC_API_URL first, llama-index reads OPENAI_API_BASE and never OPENAI_BASE_URL. Set every spelling, with a table of which wins. A source that builds the HTTP call itself has no hook at all -- that is now report-and-stop. "Streaming is unsupported" was too broad: buffered consumption works, token-by-token does not. global_controller.yaml (3 findings / 3 repos). No complete example existed anywhere. Added one, plus why `database` stays out. Unclustered: asyncio bridge granularity and loop-bound state, probe 2 not reproducing _load_agent's module naming, --env-file as the common case not an aside, session state inside `query`, and the StateGraph rewrite being conditioned on crossing a service boundary. Not closed: non-.py assets (cluster 2, CAN-282). The sweep is .py-only, troubleshooting.md describes it as conditional on a capability that has never existed, and validate.py probes a function ventis does not define. Deferred deliberately: core is taking the second half of can-228, and writing ".py only" now would be wrong as soon as it lands. All three descriptions get aligned in one pass then. This edit changes the skill's tree hash, so it forks the corpus experiment: the 45 rows pinned to skill_sha c249d63 stop being comparable with anything run after it.
SKILL.md had grown to 509 lines and step 3 was 213 of them -- the entrypoint gates, async bridging, session state, the requirements procedure and the full manifest were all inline. That is reference weight sitting in the file that loads on every invocation. Split along the boundary the port already has, one file per authored directory: references/adapter.md what goes into .car/app references/manifest.md what goes into .car/config SKILL.md keeps what has to survive a skim: the name binding, the workflow rules and their two code snippets, and the hard-rules table, which is the one-line-per-rule index into both new files. The reference list now separates mandatory from triggered. Every other reference fires on a symptom -- a failed build, a nested import -- but the rules in these two build green and fail in a container, so a porter cannot know to look them up. Their trigger is step 3 itself, named on the checklist line. 509 -> 364 lines, no content dropped.
The reference index was a flat list whose entries all read as "read this
when..." while the triggers were not the same kind of thing at all. Grouped
into four, because which group a reference is in decides when it gets opened:
Before you write adapter.md, manifest.md -- triggered by the step
When the target has packaging.md, llm-proxy.md, ec2.md -- triggered by
this shape a fact knowable at step 1
After something failed troubleshooting.md, runtime-contract.md --
triggered by a symptom
For orientation example-port.md
Each entry now says what is in the file, so the file does not have to be
opened to find out whether it is the right one.
llm-proxy.md crossed 100 lines when the env-var table went in; added the
Contents section the length calls for. Every reference over 100 lines now has
one.
Verified mechanically: SKILL.md body 370 lines, name 24 chars, description 618
chars, no reference reachable only through another reference, no
Windows-style paths.
validate.py stays at the skill root rather than moving to scripts/. The
convention fits a skill with several scripts; this one has exactly one, and
the corpus harness invokes it by that path
(canyonos-core-porting-tests/.claude/skills/testing-porting-to-canyonos-core/SKILL.md:106).
Moving one file to satisfy a directory convention is not worth breaking a
caller in another repo.
BASE_AGENT_REQUIREMENTS and BASE_WORKFLOW_REQUIREMENTS now come from the importable stub_generator, with the literals kept only as a fallback for a machine where ventis is not importable. This is what the file already claims to do for capabilities -- probe the runtime, don't restate it -- and the copy had already drifted: it said `psycopg` where the generator says `psycopg[binary]`. The failure mode is not hypothetical. `sweeps_all_files` probes `_sweep_project_files`, a function ventis has never defined, so the capability reported `no` across all 45 corpus runs and nobody noticed. Checking the other two probes the same way turns up a second instance: `editable_install` probes `_install_step`, which exists on no branch, and the generated Dockerfile runs no editable install at all -- so `no` is the right answer reached the wrong way, and the probe would not notice the capability landing. `env_file` is the one that works, probing `resolve_env_file` which PR #53 defines. Both stale probes are left as found rather than rewritten. packaging.md's "report a capability blocker and stop" and M24's "when editable install is supported" describe a capability that has never existed in any form -- the same shape as the sweep half-state in CAN-282, and the same call: report it, don't paper over it. Also fixes the one ruff finding in validate.py (FURB188), which predates this branch. The tests covering all of this live on nickhuo/close-porting-skill-gaps, out of scope for this skill-only PR.
…alidator SKILL.md was carrying three things that only matter once: the step-1 survey list, the rules for refreshing an existing `.car/app`, and adapter prose that adapter.md already gates. All three move out, and SKILL.md keeps the decision and the pointer. - references/source-survey.md: the survey, read after preparing the copy and before choosing service boundaries. - references/refresh.md: read when `.car/app` exists and the source moved on. prepare.py grows `--refresh`, which updates source-owned files while preserving port edits and stops atomically when both sides changed one path -- the case that silently loses an adapter. It now also rejects symbolic links, which either escape the self-contained artifact or are skipped by the runtime sweep, so neither outcome is worth carrying. validate.py splits into a validation package rather than growing further: core.py result and YAML primitives manifest.py the public artifact contract, fail-closed python_source.py static source discovery for adapter and packaging checks runtime.py runtime capabilities and dependency facts references/manifest.md gains the `canyonos config` View/Change review flow and re-scopes its opening: the validator checks YAML structure and the artifact contract before an approved deploy, so this reference explains how to derive the values rather than what the build will reject. (cherry picked from commit d2c82d6bfab4e0ffcb1d12b64b2470eb529be981)
…r exists
validate.py still held nineteen of the twenty-five checks after the last split,
so `validation/` was a helper package with one very large caller rather than a
set of checks. The remaining check functions move out by the section boundaries
the file already had:
validation/adapter.py V006-V010 adapter faults _load_agent swallows
validation/workflow.py V016-V018, V023 the workflow and how it
reaches an agent
validation/entrypoint.py V019, V020, V033-V035 traps set by which
module the entrypoint names
validation/packaging.py V030-V031 credentials and import roots
validation/dependencies.py W003, W006 credentials and imports a
successful build does not reject
`module_path` and the two import resolvers join python_source.py, which is
where the other static-source helpers already live; both resolvers were private
and are now shared by packaging.py and dependencies.py, so they lose the
underscore. validate.py keeps argparse, the driver, and report printing: 1153
lines to 311.
This is a pure move. All twenty-four relocated definitions are AST-identical to
their originals, and ten fixtures covering every check code that lives outside
manifest.py -- V002, V006-V010, V016-V020, V023, V030-V035, W003, W006 --
produce byte-identical `--json` output before and after.
Four references still cited hard-rule labels by number:
references/adapter.md M18
references/llm-proxy.md M18, M21
references/manifest.md M22, M23
The M1-M32 table went away when SKILL.md gained `## Source-integrity
boundary`, which restates the four rules static analysis cannot prove and
drops the twenty-eight the validator now checks itself. The rules survived; the
labels did not, so each citation named a table a reader could not find. They
now name the boundary.
Also drops nine dead `# noqa: E402` directives and sorts the new modules'
imports, which nets the skill's scripts down from twelve ruff findings to
seven. The seven that remain are in prepare.py and runtime.py and predate this
change.
(cherry picked from commit 7cfbece8cb84a2bb591aa88d346a4742a95fb540)
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| header = f"{finding['check']} {finding['level']:<5}" | ||
| print(f"{header} {where}" if where else header) | ||
| for line in _wrap(finding["summary"], 78, " "): | ||
| print(line) |
Comment on lines
+291
to
+300
| json.dumps( | ||
| { | ||
| "artifact_root": artifact_root, | ||
| "capabilities": capabilities, | ||
| "errors": errors, | ||
| "warnings": warnings, | ||
| "findings": report.findings, | ||
| }, | ||
| indent=2, | ||
| ) |
nickhuo
changed the title
iidsample
approved these changes
Sep 8, 2026
| import sys | ||
|
|
||
|
|
||
| RUNTIME_FLAT_NAMES = frozenset( |
Member
There was a problem hiding this comment.
What are these validation checks ? How do they run ?
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
3 participants
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.
No description provided.