Conversation
70fbadd to
1d90381
Compare
208decb to
62f5047
Compare
00b9f56 to
3fc4294
Compare
|
PR-cycle state — maintained automatically. Do not edit. {
"approval": {
"approved_revision_fingerprint": "sha256:96bceb114d2a20de4b6a64b74efa439c9bcb54e8be0e4466678646caeb05cfdb",
"evidence": {
"delivery_deferral_ledger_digest": "sha256:463836b313102d81e1f86c485f1f3504803966e45ebd1419f2641223a5491e88",
"required_checks": [
[
"Linux CI checks",
"success"
],
[
"Target smoke (darwin-amd64)",
"success"
],
[
"Target smoke (darwin-arm64)",
"success"
],
[
"Target smoke (linux-amd64)",
"success"
],
[
"Target smoke (linux-arm64)",
"success"
],
[
"Windows host smoke (windows-amd64)",
"success"
],
[
"Windows host smoke (windows-arm64)",
"success"
]
],
"review_request": {
"body_digest": "sha256:98630a5a0d3926bfb4b32b861173ca036161049c1310c4739c66e1690ad05c99",
"id": 5327924305
},
"review_result": {
"body_digest": "sha256:eb167be83d4b20fc0592a037819adaa02b0b35048825c2a8dc0694f89985a6cb",
"id": 5327958886
},
"revision_fingerprint": "sha256:96bceb114d2a20de4b6a64b74efa439c9bcb54e8be0e4466678646caeb05cfdb",
"thread_dispositions": {
"resolved": 9,
"total": 9
}
},
"evidence_fingerprint": "sha256:60919fb7a0c99e6baf14bfb844ef6ae447278f5f7515e68e0a3e0596f342c7e7",
"rounds": 7
},
"checks": {
"conclusion": "success",
"count": 7
},
"delivery_deferrals": [
{
"claim": "embedded payload, binding-artifact, tool, package-set, and source definitions must declare the fields added by this slice",
"disposition_history": [
"opened by PR 89 round 1: additive work outside PTD-02's scope",
"extended by PR 89 round 2: binding artifacts also need name and ecosystem_version"
],
"evidence": [
"PTD-02 adds Resolver to BindingArtifactRecordV1 and PayloadRecordV1 per design 316, 332, 367",
"strict decoding requires every non-omitempty field to be present",
"parked java payload jdk-linux-amd64.json has no resolver key",
"parked playwright binding artifact linux-amd64.json has no resolver key"
],
"key": "sha256:1d3c34a3d654891c",
"owner_tasks": [
"PTD-10",
"PTD-11"
],
"source_head": "3306e99721e5ab6d8282fb45e182fd02b40ebaa8",
"source_review_identity": "PR 89 inline finding, thread on records.go:225",
"status": "open"
}
],
"findings": [
{
"classification": "current-slice defect",
"disposition": "fix in slice: add the resolver primitive to binding-artifact and payload records",
"evidence": [
"BindingArtifactRecordV1 and PayloadRecordV1 carry Size and SHA256 but no Resolver",
"only ArtifactSourceRecordV1 declares Resolver",
"PTD-09 acceptance: validation and source-only data do not affect selected identity",
"so a resolver change on a source record alters acquisition behaviour while leaving the closure identity and cache key unchanged"
],
"id": "resolver-identity-on-artifact-records",
"location": "internal/toolcatalog/records.go:225",
"named_invariant": "the artifact record's size and resolver primitive govern every mapped locator (design 363-367); binding-artifact and payload records own the reviewed resolver primitive (design 316, 332)",
"severity": "P1",
"source_head": "3306e99721e5ab6d8282fb45e182fd02b40ebaa8"
},
{
"classification": "current-slice defect",
"disposition": "fixed in 60f43d377257; thread resolved",
"id": "component-coordinates-on-binding-artifacts",
"location": "internal/toolcatalog/records.go:187",
"named_invariant": "binding artifact records own component name and exact ecosystem version (design 311, 1086-1088)",
"severity": "P2"
}
],
"findings_summary": {
"all_applied": true,
"class": "design elements the record model did not declare",
"items": [
"resolver primitive on binding-artifact and payload records",
"component name and ecosystem version on binding artifacts",
"documentation metadata on the tool record",
"validation-evidence construction and clone coverage",
"package-set repositories and validation metadata",
"binding-contract wheel tags",
"artifact-source diagnostics",
"binding-contract bundled constituents",
"digest-checked contract reference on binding artifacts"
],
"total": 9
},
"pull_request": {
"base": {
"ref": "pr87",
"sha": "30e47213b9be0046677f5e2b67c7b53c76fcba0a"
},
"head": {
"ref": "pr89",
"sha": "97ae8d8f7daf"
},
"number": 89,
"repository": "github.com/omry/reploy"
},
"review": {
"request": "@codex, regular review 3306e99721e5ab6d8282fb45e182fd02b40ebaa8",
"result": "Codex Review with 1 inline finding \u2014 reviewed commit 3306e99721"
},
"revision_fingerprint": "sha256:96bceb114d2a20de4b6a64b74efa439c9bcb54e8be0e4466678646caeb05cfdb",
"rounds": [
{
"attempt": 1,
"checks": "focused, nox -s go-test, gofmt, vet \u2014 pass",
"finding": "resolver identity on artifact records (P1)",
"fix": "Resolver added to BindingArtifactRecordV1 and PayloadRecordV1",
"resulting_head": "120f49f84563",
"reviewed_head": "3306e99721e5",
"sync": "safe predecessor rewrite; 14 warnings all the allowed protected-base rejection, captured verbatim"
},
{
"attempt": 2,
"checks": "focused, nox -s go-test, gofmt \u2014 pass",
"finding": "component coordinates on binding artifacts (P2)",
"fix": "Name and EcosystemVersion added to BindingArtifactRecordV1",
"resulting_head": "60f43d377257",
"reviewed_head": "120f49f84563",
"sync": "safe predecessor rewrite; no warnings outside the protected-base class"
}
],
"scope_authority": {
"docs/PORTABLE_TOOL_DEFINITION_DESIGN.md": "sha256:4c8fa535721baf22f67486a46b0b4a04eb7054713d0a892e2d6f9667bba914b6",
"docs/PORTABLE_TOOL_DEFINITION_IMPLEMENTATION_PLAN.md": "sha256:971113afcedea93da5df3fab42433a1e5f9fea62554f02a3f630e306b9455f00"
},
"version": 5
} |
![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: 3306e99721
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 120f49f845
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60f43d3772
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90b78c5094
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9762bf2776
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c275560da0
ℹ️ 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".
Add the intermediate internal/toolcatalog package with the v1 schema names and the immutable record model: tool, release manifest, release contract, target, binding contract and artifact, payload, artifact source, native package set, integration fixture, validation profile, and validation evidence records, together with references, target identities, and the plural binding, selection, and typed-parameter contribution shape. Add immutable-value clone helpers and canonical string comparison helpers, plus construction coverage for every record family. Correct the clone helpers to copy every mutable field. The parked implementation copied only some of them, so a record handed to a caller still aliased loaded catalog state through target bindings, target parameters, target exports, integration fixtures, selection package sets, exports and probes, contract parameters and their default pointers, contract compatibility groups, and fixture parameters. The design treats definition records as immutable, so the independence is now proven by test. Delivers PTD-02 of docs/PORTABLE_TOOL_DEFINITION_IMPLEMENTATION_PLAN.md.
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Add the intermediate
internal/toolcatalogpackage with the v1 schema names and the immutable record model: tool, release manifest, release contract, target, binding contract and artifact, payload, artifact source, native package set, integration fixture, validation profile, and validation evidence records, together with references, target identities, and the plural binding, selection, and typed-parameter contribution shape.Add immutable-value clone helpers and canonical string comparison helpers, plus construction coverage for every record family.
Scope authority
Task: PTD-02 — Define Portable Tool Record Data Model, defined in docs/PORTABLE_TOOL_DEFINITION_IMPLEMENTATION_PLAN.md (PR #86 in this stack).
Acceptance criteria:
Truth fix: records handed to callers must not alias catalog state
The parked clone helpers copied only some mutable fields, so a resolved record still aliased the shared loaded catalog through target bindings, target parameters, target exports, integration fixtures, selection package sets, exports and probes, contract parameters and their default pointers, contract compatibility groups, and fixture parameters.
Local review found the fix was itself incomplete:
ValidationProfileRecordV1had no clone helper at all, and the parked resolver assigns it into the selected closure as a bare*profilewhile cloning every other record. ItsProbesand theirArgstherefore aliased the catalog.cloneValidationProfileV1is added here, and PTD-09's acceptance criteria now require every record placed in a closure to be cloned.Independence is proven for all eleven helpers by a reflection-based test that writes through every string reachable in a clone and asserts the original is unchanged. The test was itself verified by breaking two helpers and confirming exactly those two subtests fail.
Remote review finding, applied
Resolverwas declared only onArtifactSourceRecordV1. The design assigns the reviewed resolver primitive to binding-artifact records (docs/PORTABLE_TOOL_DEFINITION_DESIGN.md:316) and payload records (line 332), and states that the artifact record's size and resolver primitive govern every mapped locator (line 367). Because source records are excluded from selected-closure identity, a resolver change on a source record would alter acquisition behaviour while leaving the closure identity and cache key unchanged — one cache key covering two materialization behaviours.BindingArtifactRecordV1andPayloadRecordV1now carry the field.Deferred to PTD-10 and PTD-11: the embedded Java and Playwright definitions declare no
resolverkey on their payload and binding-artifact records, and strict decoding requires it. Recorded in the PR-cycle state comment.Exclusions: decoding, semantic validation, graph traversal, resolution. Structural and validation limit constants arrive with the slices that consume them. Sources: parked extraction sources
b39985d247e5and37ca781bd6cb.