Repository navigation
docs: settle the workflow sidecar shape and let sidecars supply recommendations - #243
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Skill evaluationAutomatic execution / cumulative PR budget: Advisory: obtain missing evidenceRetained evidence availability: planned / unexecuted. No approval or merge authorization is issued.
PLANNED: execution is not authorized by knowing this hash. Selected comparisons: 0 · Estimated task calls: 0. Applicable only while these revisions and inputs match the current PR. Plan bindings, budget, frozen exam and advisory detailsUnchanged-correct is valid neutral evidence, not incremental benefit. Per-case outcomes (discovery excluded; regressions and gaps first): []Showing at most 20 of 0 pairs; the evidence link contains all outcomes, including omitted regressions/gaps. Plan SHA-256: Model/settings:
Provider/settings: Uncovered / unsupported paths (94)
|
Dong1017
left a comment
Dong1017
left a comment
There was a problem hiding this comment.
Integration preflight at head 1b7868365b1cc4192bc63a99ba037bd96a3e76d0, API base and observed main a0a76b33de93bc131af5849b519b58ae077aa6d2: hold for integration, not merge-ready.
GitHub reports mergeable=false / dirty. The current-head check inventory contains declaration success and Skill Eval plan/publish success, with execute skipped; no Ready full-check or installed-package results were found for this SHA. Successful planning/admission does not replace those checks.
Next owner/action: the existing PR/#91 owner should reconcile current main while preserving both sets of canonical sources, regenerate the affected discovery index and coverage view with the existing tools, refresh the truthful declaration for the resulting head, and obtain applicable Ready results. Specific conflicting files have not been reproduced locally. Re-review the resulting integration delta rather than treating the PR API base pointer as proof that it is integrated.
Verification: live refs, governing #91 discussion, changed-path inventory, review threads and exact-head checks only. This is an integration-status comment, not a source-design LGTM, batch acceptance or efficacy/promotion result; no PR code, generator, model or hardware execution.
Dong1017
added
the
area:knowledge
ChatGPT Codex Connector
1b78683 to
55fbfb4
Compare
Dong1017
left a comment
Dong1017
left a comment
There was a problem hiding this comment.
Blocking review at 55fbfb4 against 861a2bb. Prior conflict is cleared, but this revision is not merge-ready. COMMENT is used because the authenticated account is also the PR author and cannot submit a self REQUEST_CHANGES review; the finding remains blocking.
[P2] Keep invalid applicability isolated to its own Collect omission. src/inferrsi/skill_catalog.py:213–215 invokes the new recommendations/applicability validators during skill_records(). KnowledgeReader.collect() calls owners() before per-item error handling; owners() inventories every checkout skill through skill_records() (knowledge_operations.py:269,515). One malformed applicability field therefore aborts the whole collection, including unrelated valid requested skills, instead of producing the invalid owner's omission.
Existing tests/test_knowledge_operations.py:363–374 supplies applicability: &loop [*loop]. Current CI raises ValueError('Skill applicability must be a mapping: sk') rather than returning checkout:ok plus an invalid omission for sk. Preserve strict metadata validation while separating identity enumeration from per-resource validation, or otherwise preserve per-owner invalid results. Keep the mixed valid/invalid regression and cover malformed new recommendations/applicability fields; do not remove the omission assertion or make malformed metadata valid.
Ready https://github.com/InferMatrixLab/InferRSI/actions/runs/40827730489 fails both full-check jobs; both installed-package jobs pass. Python3.11 reports 1 failed,2479 passed,11 skipped,259 subtests on tested merge1c325b3ce135b9a3d3efac6c09a31a5f4b6a351b combining this head and stated base. Correct the regression and obtain applicable checks on the resulting head.
Verified via source/caller/test inspection and hosted logs, no local PR/model/hardware execution. This clears only the old conflict observation, not this blocker, #91 acceptance or efficacy.
8e86c67 to
417c677
Compare
Dong1017
left a comment
Dong1017
left a comment
There was a problem hiding this comment.
Incremental blocking follow-up at 417c67798c9d4a939019fcd007f17ee7b21556f7 (observed base/main a8215b3ec314b8bc5cee152cdebbf4bb76ba4188). This is the remaining part of review 5694024409, not a new requirement. COMMENT is used because the authenticated account is also the PR author and cannot submit self REQUEST_CHANGES.
[P2] Malformed sidecar fields still abort unrelated Collect requests. Moving validation into the sidecar branch avoids the old legacy cyclic-YAML fixture, but skill_catalog.py:209–214 still raises during global owner enumeration. With valid sk and ok owners and a sidecar on sk containing either {"applicability":[]} or {"recommendations":"bad"}, both collect(ids=["sk","ok"]) and collect(ids=["ok"]) raise instead of preserving the valid owner. owners() calls the catalog before Collect's per-item error handling.
Smallest scoped correction: retain strict field validation but defer/isolate these content failures so the affected requested owner becomes an invalid omission, without aborting valid neighbors. Add mixed valid/invalid sidecar tests, including an invalid unrequested owner. Do not weaken validation merely to pass the fixture.
Verification: exact-head source extraction with isolated temporary fixtures reproduced the two field failures; no repository commands, installed dependencies, model or hardware experiments were run. Current declaration and SkillEval planning/publication pass; execution is skipped and no exact-head Ready results were found. GitHub still reports a merge conflict. This is not LGTM or #91 efficacy/goal acceptance.
417c677 to
dff0f25
Compare
Dong1017
left a comment
Dong1017
left a comment
There was a problem hiding this comment.
Re-review at head dff0f258669091dd75613aa9899d28a638c4921c, API base 5a46ad5a9e5bb93ec8111d47d97c7e341c476710, observed main a4ed530923fe59e9e5b0e9f44e1bf3fd303b84c5: hold for the existing P2; no LGTM.
Newly cleared: the merge conflict and missing Ready evidence. All four current-head Ready jobs pass. Inspected Python3.11 logs show Ruff success, 2511 passed, 11 skipped and 284 subtests on merge c3919f5 of this head into the stated base. The latest per-head policy succeeds; the earlier same-head declaration failure is superseded.
The remaining P2 from review5695248739 still applies. The catalog patch is byte-identical to417c: sidecar validation still runs through global skill_records()/owner enumeration before Collect's per-item handling. Thus an invalid unrequested owner's new fields can still abort a valid owner's collection. No duplicate inline finding is added.
The specific legacy cyclic-frontmatter failure is cleared, with its original mixed-owner fixture preserved. New catalog tests expect invalid sidecars to raise; they do not test valid-neighbor Collect behavior. Next owner/action: the existing author isolates content validation from identity inventory and adds requested/unrequested malformed-sidecar Collect cases while retaining strict validation.
SkillEval planning/publication succeeds with execution skipped and zero selected comparisons; this is not efficacy or #91 acceptance. Later main adds the Codex plugin without changing the assessed catalog/Collect path. Verification: static diff/caller/test inspection and hosted logs, no local repository execution, model or hardware run.
COMMENTED is used because the authenticated account is also the PR author and cannot submit a formal self REQUEST_CHANGES. This updates current-version integration/CI status and carries the existing blocker forward.
dff0f25 to
94e7249
Compare
Dong1017
left a comment
Dong1017
left a comment
There was a problem hiding this comment.
Source-focused re-review at head 94e7249c8e6f31784707d6a0cf84534e0f050569, API base a4ed530923fe59e9e5b0e9f44e1bf3fd303b84c5, observed main 7d1d82f55cda89842bd23ee2da91b521ee60eb08.
The previous P2 is cleared for this revision. owners() now inventories names through skill_directories(), leaving sidecar validation inside the requested resource's read path. The original cyclic-frontmatter fixture is retained. New tests cover mixed valid/invalid owners, an unrequested malformed owner, and strict rejection of malformed applicability/recommendations. I found no further blocking runtime correctness issue in the assessed scope.
Two nonblocking P3 presentation issues are attached: the Markdown view never consumes the collected workflow-contract/advice summary, and an N/A-only applicability declaration loses its N/A marker unless it also has scope. Discovery JSON recommendations are emitted correctly; these findings concern the reverse Markdown view.
Reviewed the changed enumeration/validation path and Collect caller, both generator diffs, related tests, the six sidecars against their procedure bodies, and generated maturity/state deltas. The five new sidecars explicitly remain candidate/not_run; no promotion was inferred.
All four Ready jobs pass. Inspected Python3.11 logs show Ruff success, 2529 passed, 11 skipped and 284 subtests on merge 98ad306 combining this head and the API base. The latest per-head policy succeeds; the earlier declaration failure is superseded. SkillEval plan/publish succeeds with execute skipped, not efficacy evidence.
GitHub still reports a conflict with actual main after #251. This review does not approve an unresolved combined tree or grant #91 acceptance. Existing owner: preserve both canonical source sets and regenerate derived outputs during reconciliation, then refresh affected checks. Verification was static source/CI-log inspection; no local repository code/tests, model or hardware execution. COMMENTED review; no formal self-approval or merge action.
94e7249 to
3c05727
Compare
Dong1017
left a comment
Dong1017
left a comment
There was a problem hiding this comment.
Incremental re-review at head 3c057270864a6be3e558a9869ec6f710d8e8810d, API base 7d1d82f55cda89842bd23ee2da91b521ee60eb08, and actual main ab03f3ffb8f2e5bc02c304828c5c41890eef673e.
Both P3 findings from review 5696532516 are cleared. The rendered Conditions column consumes the contract keys/advice count; N/A dimensions render independently of scope. The new tests inspect rendered rows, including the N/A-only case. The catalog, reader and index-generator files are byte-identical to the previously reviewed 94e7249c8e6f31784707d6a0cf84534e0f050569, so the former per-owner Collect P2 remains cleared. No additional blocking source finding in this incremental scope.
All four Ready jobs pass. The inspected Python3.11 log records Ruff success, 2531 passed, 11 skipped and 284 subtests. Checked content was merge 9efabecbac227b88a91f74039da5ef14534a2455, whose parents are the stated API base and reviewed head. The latest per-head declaration policy succeeds and supersedes the earlier same-head failure. SkillEval planned/published successfully, with execution skipped and zero selected comparisons; its 93 uncovered paths are not efficacy passes.
Hold for integration with actual main; no merge-ready LGTM. The API still reports clean against the older base, but the tested merge does not contain the new Lithos knowledge delivery on actual main. Both sides rewrite the same generated provenance lines: for example, assess-serving-preemption.json changes sources[0].commit from 2f72a0ccabd87eefdc76223c9b82f7091c379da1 to b6ae0561e9091b5428cfed0f917eba0c27b5a12c in this PR, versus 26743abcfae48cd6223bd757e74f193d97e3e5a7 on main. See the PR source delta and main advance. These need reconciliation rather than treating the older clean merge as current-main evidence.
Next for the existing PR/#91 owner: preserve the canonical Lithos and workflow sources, reconcile current main, regenerate discovery/coverage outputs, update the truthful declaration for the resulting head, and refresh affected checks. This clears the reviewed source findings; it does not accept the wider #91 tracker or demonstrate model/hardware benefit.
Verification: source/blob comparison, existing discussion and hosted CI logs only; no local repository code/tests, model or hardware execution. COMMENTED is used because the authenticated account is also the PR author and cannot submit formal self-approval. No merge action.
Dong1017
left a comment
Dong1017
left a comment
There was a problem hiding this comment.
增量静态 review（head 3c057270864a6be3e558a9869ec6f710d8e8810d；base 7d1d82f55cda89842bd23ee2da91b521ee60eb08）：本轮仅发现一项未被先前讨论覆盖的 P3 适用范围呈现不一致，已定位到新增的 conditions() 行内。此前两项 Markdown 呈现意见已有修复回复，本轮不重复；不评价整个 #91 的交付或合入状态。依据是当前 diff、skill_catalog._applicability 允许值形状、调用链及覆盖视图测试的静态检查；未运行代码、测试、模型或硬件实验。
Dong1017
left a comment
Dong1017
left a comment
There was a problem hiding this comment.
LGTM with nits for the reviewed source changes at 3c057270864a6be3e558a9869ec6f710d8e8810d.
Please address the remaining nonblocking list-valued scope rendering issue, resolve the conflicts with main ab03f3ffb8f2e5bc02c304828c5c41890eef673e, and regenerate the affected index/coverage outputs. Refresh the applicable checks on the resulting head and recheck the integration delta.
The assessed API base is 7d1d82f55cda89842bd23ee2da91b521ee60eb08; detailed source and existing-CI evidence is in the preceding review. The nit is nonblocking for this source-review conclusion. The conflicted combined tree still needs the author's integration work before merge.
…mendations Batch 1 of the InferMatrixLab#91 Round 2 plan settles the two contract decisions that every later batch depends on, and proves them on the six registered `workflows/` skills. Reuse the existing sidecar envelope rather than adding a parallel record system. `applicability` gains four keys that describe a procedure instead of hardware: `input_contract`, `produced_artifact`, `failure_modes` and `refusal_point`. No workflow sidecar declares `hardware`, because a process skill has no device dimension. The five skills that had no sidecar and the existing `npu-skill-factory` sidecar now share one shape. `recommendations` becomes a sidecar field. The generator emitted the same two sentences for all 77 records and no owner could contribute. It now emits the owner's list when present, keeps the previous text when the field is absent, and honours an explicit `[]` without falling back: an empty list states that there is no procedure-specific advice. The coverage view now collects `applicability` and distinguishes three states that previously rendered as one blank cell: absent or empty (`unknown`, counted as a new `no-applicability` gap), a declared scope, and a dimension marked `not-applicable`, which is a reasoned statement and does not count as coverage. Two columns carry the result. `Applicability` holds the declared scope, and `Conditions` renders the typed workflow keys with the collected advice count, so changing a contract field or an owner's recommendations is visible in the row rather than only in the document digest. N/A renders independently of scope: a declaration that marks a dimension not-applicable keeps that marker even when it names no scope. A list-valued scope, which the catalog accepts like any other applicability key, renders as its joined entries rather than as missing. `skill_catalog` validates both new fields on the sidecar. The check runs when an owner is read, not while identities are enumerated: `owners()` now takes registered names from `skill_directories()`, which resolves paths and entry files only. A malformed sidecar therefore reaches Collect's per-item handler and becomes that owner's `invalid` omission instead of aborting unrelated requested owners. Validation itself is unchanged, so a malformed owner still fails. `tags` keeps its existing contract on the effective record. Generating the index also required a pre-existing Windows fix: `build_index` passed all 545 skill input paths on one command line to `git diff`, which exceeds the process-argument limit and failed with `WinError 206`. It now compares the whole-tree diff and intersects in Python. No evaluation, verdict or promotion. `status` stays `candidate` and `empirical_validation` is unchanged for all 77 skills. The 41 empty `applicability` fields belong to batches 2 through 4; layer decisions are batch 5. Agent-assisted-by: Claude / Opus 5.5 [effort=xhigh] (sidecar shape, generator and view changes) Signed-off-by: Dong1017 <55723162+Dong1017@users.noreply.github.com>
3c05727 to
5076062
Compare
Batch 1 of the #91 Round 2 plan
Round 1 (merged in #194) built the reverse coverage view with a row per
registered skill id. Every field it could not answer said
unknown, and fiveworkflow skills had no sidecar at all. This batch settles the two contract
decisions that later batches depend on, and proves them on the six workflow
skills.
Plan and review: #91 Round 2 scope and plan.
Decision 1 — reuse the sidecar envelope, add typed workflow conditions
No parallel record system.
applicabilitygains four keys that describe aprocedure rather than hardware:
input_contractproduced_artifactfailure_modesrefusal_pointscopeandprerequisiteskeep their existing meaning. No workflow sidecardeclares
hardware, because a process skill has no device dimension.The five new sidecars and the existing
npu-skill-factorysidecar now share oneshape, so the six registered
workflows/skills are consistent rather thanfive-plus-one.
Decision 2 —
recommendationsbecomes a sidecar fieldtools/build_skill_index.pyemitted the same two sentences for all 77 recordsand no sidecar could contribute. It now calls
recommendations(metadata):[]An explicit empty list is a statement that there is no procedure-specific
advice, so it must not silently fall back to the generic text.
Decision on
not-applicableversus blanktools/build_skill_coverage.pynow readsapplicabilityand distinguishesthree states that previously all rendered as a blank cell:
{}**unknown**no-applicabilitynot-applicablevalueN/A: <dimensions>The blanket value is
not-applicable. A dimension marked that way is areasoned statement, not a missing field, and it does not count as coverage.
The view now exposes the fields it is credited with
The plan review noted that the coverage view did not collect
applicabilityorrecommendationsat all, so filling the empty sidecars would have changed nothing inthe table. The generator now collects both and adds two columns to every
registered table, plus
no-applicabilityto the gap legend and therecorded-gaps table:
catalog accepts, renders as its joined entries. A dimension marked
not-applicablerenders asN/A: <dimension>independently of scope, so anN/A-only declaration keeps its marker instead of collapsing into
— (declared, no scope).prerequisites, plus the advice count. Changing a workflow contract field or an
owner's recommendations therefore changes the visible row.
recommendationsis counted (N advice) rather than rendered in full: 77 rows ofprose would not fit the table.
Validation rules added
skill_catalog._effective()now rejects malformed values before any generatorreads them:
recommendationsmust be a list of non-empty strings;[]is allowed.applicabilitymust be a mapping; each value is a non-empty string or anon-empty string list.
These run on every sidecar in the library, not only the new ones.
Validation stays strict, but it no longer runs during identity enumeration.
KnowledgeReader.owners()now reads registered names throughskill_directories(), which resolves paths and entry files only. A malformedsidecar therefore fails when Collect reads the owner that declares it, through
the existing per-item handler, and becomes that owner's
invalidomission. Anunrequested malformed owner no longer aborts a request for a valid one, and
collect(ids=["sk", "ok"])with a brokenskreturnscheckout:okplus aninvalidomission forsk.Scope accounting at this revision
Counts read after rebasing onto
main=ab03f3ff:metadata.jsonsidecarsapplicabilityabsent or emptySKILL.mddocumentsThe 41 empty
applicabilityfields belong to batches 2 through 4(
methods17,hardware20,repositories3,frameworks1). This batch addsthe keys and the rendering, not the content.
Incidental fix required to generate the index
build_index()passed every skill input path on one command line togit diff. At 545 inputs that exceeds the Windows process-argument limit, sotools/build_skill_index.pyfailed withWinError 206on Windows. It nowcompares the whole-tree diff and intersects in Python. The check is equivalent
and the error message is unchanged. This is a pre-existing defect, not
introduced here.
Checks
ruff check src tests toolsclean.tests/test_skill_coverage_view.py,test_skill_catalog.py,test_knowledge_contract.py,test_skill_index.py,test_knowledge_operations.py: passing.not-applicableseparation, gap classification,workflow row shape, recommendation fallback and explicit-empty behaviour, and
that filling conditions never raises
locally_verifiedor changesempirical_validation.and a list-valued scope through
collectandrender.test_knowledge_operations.pycovers a malformed sidecar as a per-ownerinvalidomission, including an unrequested malformed owner.docs/skill-coverage.mdregenerated in the same commit as its sources.Not claimed
No evaluation, no verdict, no promotion.
statusstayscandidatefor all 77skills and
empirical_validationis unchanged.no-evalstill applies to 75rows and belongs to Round 3. No matrix row was added or edited; layer decisions
are batch 5.
Refs #91
Protected path
src/inferrsi/data/repo_packs/inferrsi/knowledge/holds 72 generated files inthis diff. They are the output of
tools/build_skill_index.pyand were producedin the same commit as their sources under
data/skill_library/; no record washand-edited. The change to those records is confined to
skill_metadata.recommendationsand the six workflow skills'skill_metadata,which now carry the new sidecar fields.
The declaration below is required because the path matches
repo_packs/**/knowledge/**.Refs #91, batch 1 of 5