release: Skill Finder v1.3.0 hybrid evidence engine - #11
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 245a783382
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public = bool(case.get("public", True)) | ||
| material = int(case.get("material_claims", 0)) | ||
| verified = int(case.get("primary_verified_claims", 0)) |
There was a problem hiding this comment.
The evaluation runner copies completed, verification counts, recovery outcomes, and safety outcomes directly from each case fixture instead of invoking the evidence engine. Consequently, the CI release thresholds remain perfect even if planning, recovery, evidence validation, or prompt-injection handling regresses, because editing implementation code cannot affect these results. Derive these fields by running the scenario through the engine rather than treating expected outcomes as observed outcomes.
Useful? React with 👍 / 👎.
|
|
||
| PRIMARY_SOURCE_TYPES = frozenset( | ||
| { | ||
| "source_code", |
There was a problem hiding this comment.
The two committed clean-room bundles label every material source verification as repository_source (and evaluation/test_harness.py explicitly requires that value), but this whitelist only recognizes source_code. Passing either bundle's records to EvidenceLedger.validate_material_claims() therefore marks every material claim unverified, preventing the advertised repository-heavy Deep Evaluation output from satisfying its evidence floor.
Useful? React with 👍 / 👎.
| candidates = [ | ||
| route | ||
| for route in route_list | ||
| if route.route_id in available_routes | ||
| and route.route_id not in attempted_routes | ||
| and route.family != failed.family |
There was a problem hiding this comment.
When available_routes includes a connected route that is not authorized, or a route incompatible with a private, secret, or local-only request, this selection can still return it solely because it has the lowest priority in a different family. Recovery can therefore cross the account and data-handling boundaries enforced by route_can_handle() during initial planning; recovery selection needs the request/capability policy checks as well.
Useful? React with 👍 / 👎.
| all_ids = {candidate_id, *(aliases or set())} | ||
| canonical = next( | ||
| (self._aliases[value] for value in all_ids if value in self._aliases), | ||
| candidate_id, |
There was a problem hiding this comment.
When a newly added alias bridges two existing candidates, all_ids contains aliases for both canonicals and this next() chooses whichever set element is encountered first. Python's set order varies with the hash seed, so the same evidence can retain different candidate IDs across runs (for example, merging aliases for alpha and beta alternates between those IDs), undermining deterministic bundles and stable deduplication.
Useful? React with 👍 / 👎.
| "route_id": {"type": "string"}, | ||
| "source_family": {"type": "string"}, | ||
| "source_type": {"type": "string"}, | ||
| "source_updated_at": {"type": "string"}, |
There was a problem hiding this comment.
Both committed observed-use bundles include source_updated_at: null for evidence whose source update time is unknown, but this contract permits only strings. Consumers validating those published evidence.jsonl files against the shipped schema will reject them; either allow null here or omit/normalize the field consistently in the bundles.
Useful? React with 👍 / 👎.
| if revision.strip(): | ||
| normalized += f"@{revision.strip().lower()}" |
There was a problem hiding this comment.
Git branch and tag names are case-sensitive, so lowercasing the revision makes distinct refs such as Release and release share one canonical identity and can describe a revision that does not exist. Repository host/path normalization can remain case-normalized where appropriate, but the supplied revision must be retained verbatim.
Useful? React with 👍 / 👎.
| eligible = sorted( | ||
| ( | ||
| route | ||
| for route in routes | ||
| if route_can_handle(route, request, snapshot) | ||
| ), | ||
| key=lambda route: (route.priority, route.lane == "connected", route.route_id), |
There was a problem hiding this comment.
The sort orders by numeric priority before lane, so the current sourcegraph connected route at priority 50 is considered before the public ecosystems route at priority 80. For a Deep public request with local-source, authorized sourcegraph, and ecosystems available, the planner selects local-source plus sourcegraph and stops even though the public route can provide the required second source family, contradicting the rule that connected routes are used only when public/local evidence cannot suffice.
Useful? React with 👍 / 👎.
| def _identity_payload(self) -> dict[str, Any]: | ||
| return { | ||
| "candidate_id": self.candidate_id, | ||
| "claim": self.claim, | ||
| "source_uri": self.source_uri, | ||
| "route_id": self.route_id, | ||
| "checked_at": utc_iso(self.checked_at), |
There was a problem hiding this comment.
The record ID payload omits the artifact digest, role, status, and all sub-second timestamp precision because utc_iso() truncates microseconds. Two observations of the same claim/source/route within one second can therefore receive the same record_id even when they refer to different artifacts or contradictory statuses, making route-attempt references ambiguous and breaking provenance deduplication.
Useful? React with 👍 / 👎.
| "engines": { | ||
| "node": ">=22.20.0" |
There was a problem hiding this comment.
The newly locked skills@1.5.20 package requires Node.js >=22.20.0, while the root manifest still advertises support for Node.js >=18; browse@0.9.5 also requires at least Node 20.19. Users on Node 18 or earlier Node 20 releases are therefore told their runtime is supported but receive engine warnings and an unsupported setup-tool installation, so the root engine range and setup documentation need to match the strictest direct dependency.
Useful? React with 👍 / 👎.
| - uses: actions/checkout@v7 | ||
|
|
||
| - uses: actions/setup-python@v5 | ||
| - uses: actions/setup-python@v7 |
There was a problem hiding this comment.
The validation workflow references v7 tags for both actions/checkout and actions/setup-python, but those major-version tags are not published. GitHub Actions fails while resolving the first step, so none of the newly added engine, evaluation, package-sync, or npm checks run; use existing released major tags instead.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
npm testSKILL_FINDER_LIVE_TESTS=1 python3 -m unittest validation.test_live_contractspython3 scripts/sync_plugin_package.py --checknpm ci --ignore-scriptsnpm ls --allgit diff --check