Conversation
|
You have reached your Codex usage limits for security reviews. Please try again later. |
omry
marked this pull request as draft
August 18, 2026 17:29
![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: 5798b77cad
ℹ️ 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".
Owner
Author
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Owner
Author
|
PR-cycle state — maintained automatically. Do not edit. {
"approval": {
"approved_revision_fingerprint": "sha256:65753e773ecd716fe4b2db92eec237821856bba0542e149adb8010a42322cbf5",
"evidence": {
"delivery_deferral_ledger_digest": "sha256:4f53cda18c2baa0c0354bb5f9a3ecbe5ed12ab4d8e11ba873c2f11161202b945",
"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:4dc502ad1f060a10348ffaf9211c2e02e9b1116ee6f9f9fff5c28e9948963ed8",
"id": 5332266449
},
"review_result": {
"body_digest": "sha256:a61854c64f63588c59cf212bd49429ee86f3eb85c95c1688ca0c3b2863278f85",
"id": 5332301327
},
"revision_fingerprint": "sha256:65753e773ecd716fe4b2db92eec237821856bba0542e149adb8010a42322cbf5",
"thread_dispositions": {
"resolved": 11,
"total": 11
}
},
"evidence_fingerprint": "sha256:8756bf88faabf92a16fbd1509eafe5b8cfea785b10dd31057aca5d513d5959f2",
"rounds": 5
},
"checks": {
"conclusion": "success",
"count": 7
},
"coverage": "85.2 percent of statements in records_compose.go; every function exercised",
"delivery_deferral_ledger_digest": "sha256:4f53cda18c2baa0c0354bb5f9a3ecbe5ed12ab4d8e11ba873c2f11161202b945",
"delivery_deferrals": [],
"findings": [
{
"disposition": "non-actionable in this slice; recorded as a discovery owned by PTD-06. The plan parks validateManifestResolvedGraphV1 for PTD-06 and names whole-catalog reachability a PTD-05 non-goal, so an aggregate entry point here would multiply-own the hunk.",
"id": "invoke-composition-validation-from-the-record-loading-path",
"round": 1,
"severity": "P1"
},
{
"disposition": "fixed in 20bb02614cd1; no comparison of payload platform against the target existed anywhere in the file, so an amd64 target could install an arm64 payload",
"id": "require-payloads-to-match-the-target-platform",
"round": 1,
"severity": "P1"
},
{
"disposition": "fixed in 20bb02614cd1; a chromium mapping could reference a firefox-owned payload, so selecting Chromium installed Firefox content while every check passed",
"id": "verify-payload-ownership-before-adding-a-selection",
"round": 1,
"severity": "P1"
},
{
"disposition": "fixed in 9f3c62e10d06; the parked equality check contradicted design :278, where mappings are keyed by symbols declared in the contract AND advertised by the target. Enumeration now derives from target mappings, with three guards so a subset cannot silently erase support.",
"id": "preserve-target-specific-binding-availability",
"round": 2,
"severity": "P2"
},
{
"disposition": "fixed in 9f3c62e10d06; I had keyed probes by path and invented a conflict rule the design does not state. Probe identity is the complete canonical value, so distinct argument lists are two probes.",
"id": "key-probes-by-their-complete-identity",
"round": 2,
"severity": "P2"
},
{
"disposition": "fixed in 778892f99c55; my own inconsistency, having added the guard for the default binding in round 2 and not for default selections. Narrowing coverage around the gap hid it: a target dropping a default stopped enumerating its tuples, fixtures passed, and a versionless request normalizing to that default had no mapping to traverse.",
"id": "reject-targets-that-omit-contract-default-selections",
"round": 3,
"severity": "P1"
},
{
"disposition": "fixed in 778892f99c55; contract.BundledComponents was never read, so an artifact bundling Node 23 under a contract advertising Node 24 passed. Artifacts bundling more than the contract declares are accepted deliberately, with a positive test making the choice visible.",
"id": "validate-bundled-components-against-the-binding-contract",
"round": 3,
"severity": "P1"
},
{
"disposition": "fixed in 89c8d6048e6a; unsatisfiable by construction. The empty set was enumerated whenever minimum was 0 while normalizedFixtureTupleV1 substitutes defaults for an empty fixture selection, so no fixture could ever produce that key and coverage rejected every contract with optional selections and a default.",
"id": "skip-empty-selection-tuples-when-defaults-apply",
"round": 4,
"severity": "P2"
}
],
"open_discovery": "BindingContractV1 validates Package and parses every requirement into a distribution but never checks Package is among them. The parked validateBindingArtifactAgainstContractV1 enforces this per-artifact. Whether the record-local form should replace or supplement it is not settled by this PR.",
"preflight_audit": "coverage measured before requesting review. Six functions were at 0 percent, including validateTargetFixtureCoverageV1, which implements this slice's headline acceptance criterion, and validateFixtureAgainstTargetV1. The passing suite did not exercise them. Tests were added until every function in the slice carries coverage; the file is at 83.3 percent of statements. After round 2, coverage was measured again and direct tests were added for validateBindingArtifactAgainstContractV1 and the package and export conflict branches, taking the file to 84.9 percent of statements. That audit found dead code: the moved extras-parsing branch is unreachable because PTD-01 rejects extras in a package root requirement and record-local validation rejects a contract carrying one. The branch was removed and the silent continue on an unparseable requirement now returns the error.",
"pull_request": {
"base": {
"ref": "pr93",
"sha": "e7fca0286a02"
},
"head": {
"ref": "pr94",
"sha": "89c8d6048e6a"
},
"number": 94,
"repository": "github.com/omry/reploy"
},
"review": {
"request": "@codex, regular review 89c8d6048e6a3524e37ec2cb0d6d0bec181b45d8",
"result": "Codex Review - no issues on 89c8d6048e"
},
"revision_fingerprint": "sha256:65753e773ecd716fe4b2db92eec237821856bba0542e149adb8010a42322cbf5",
"rounds": [
{
"attempt": 1,
"findings": 3,
"resulting_head": "20bb02614cd1",
"reviewed_head": "6c76bc8a3ace",
"severities": "3 P1"
},
{
"attempt": 2,
"findings": 2,
"resulting_head": "9f3c62e10d06",
"reviewed_head": "20bb02614cd1",
"severities": "2 P2"
},
{
"attempt": 3,
"findings": 2,
"resulting_head": "778892f99c55",
"reviewed_head": "9f3c62e10d06",
"severities": "2 P1"
},
{
"attempt": 4,
"findings": 1,
"resulting_head": "89c8d6048e6a",
"reviewed_head": "778892f99c55",
"severities": "1 P2"
},
{
"attempt": 5,
"findings": 0,
"note": "clean",
"resulting_head": "89c8d6048e6a",
"reviewed_head": "89c8d6048e6a"
}
],
"scope_authority": {
"docs/PORTABLE_TOOL_DEFINITION_DESIGN.md": "sha256:9148c9398eb162f9597bf66d61adbbf27257b6ea6edca8d1b68e088ed04916e6",
"docs/PORTABLE_TOOL_DEFINITION_IMPLEMENTATION_PLAN.md": "sha256:ae49b856a644489e7a49d87601b12469b45cb6060e80abf49eb9c866dcd48f5a"
},
"stack_membership": "PRs 93 and 94 were base-chained but not members of the GitHub native stack, because sl pr submit creates with base main and the base was then set by REST API, which does not confer membership. Linked into stack #87 with gh stack link, after which pushes stopped resetting the base to main.",
"task": "PTD-05: Validate Target Composition and Fixture Coverage",
"truth_fixes": [
"interpreter coverage across the selected artifact set, replacing a per-artifact any-match check",
"co-selectable payload logical-path and install-destination collisions, which the parked source never checked because it never gathered payloads",
"probe identity as a semantic key under design rule 10"
],
"version": 4
} |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c76bc8a3a
ℹ️ 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".
Owner
Author
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20bb02614c
ℹ️ 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".
Owner
Author
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f3c62e10d
ℹ️ 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".
Owner
Author
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 778892f99c
ℹ️ 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 composition layer above record-local validation: one target validated against its release contract, the support tuples that target advertises enumerated under a bound, and every tuple proven to have integration-fixture coverage with no unselected contribution leaking in. Fourteen functions move from the parked extraction source b39985d247e5 without semantic change, together with the supportTupleV1 type they enumerate. maxDefinitionValidationCases already exists from PTD-04, and PackageRootDistributionNameV1 resolves through PTD-01, so nothing else is new. Three truth fixes, each required by the normative design and each carrying negative coverage. The parked source disagrees with the design on all three. A binding advertises a set of interpreters, and the parked check passes as soon as one of them is satisfied by one artifact. Advertising 3.11 and 3.12 while shipping only a cp311 wheel therefore passed, leaving an advertised interpreter with nothing to install. Coverage is now checked across the whole selected artifact set, and validateTargetBindingsAgainstContractsV1 gives both the per-artifact and the set-level check a caller in this slice. Co-selectable payloads could collide. The parked tuple check gathers package sets and exports and never gathers payloads at all, so two payloads reachable in one support tuple could share a logical path or own overlapping install destinations and still pass. Design rule 10 makes both semantic keys and allows a shared unowned parent while forbidding overlapping owned trees, which is what recordPathOverlapsV1 implements. Probe identity is the sixth semantic key in that same rule and was likewise unenforced. Identical probes deduplicate; the same executable invoked with different arguments is now a conflict rather than two probes. The composition validators have no production caller yet. PTD-06 owns the release-graph walker that calls them, so they are exercised directly by tests here rather than through a graph. One observation recorded rather than fixed: validRecordValuesV1 is not reference-closed, because the sample target names an unconditional payload the shared set does not contain. Nothing before this slice resolved references, so nothing noticed. The composition test helper supplies the missing payload instead of changing a fixture four approved slices depend on.
Owner
Author
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
This was referenced Aug 18, 2026
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
1 participant
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.
Add the composition layer above record-local validation: one target validated
against its release contract, the support tuples that target advertises
enumerated under a bound, and every tuple proven to have integration-fixture
coverage with no unselected contribution leaking in.
Fourteen functions move from the parked extraction source b39985d247e5 without
semantic change, together with the supportTupleV1 type they enumerate.
maxDefinitionValidationCases already exists from PTD-04, and
PackageRootDistributionNameV1 resolves through PTD-01, so nothing else is new.
Three truth fixes, each required by the normative design and each carrying
negative coverage. The parked source disagrees with the design on all three.
A binding advertises a set of interpreters, and the parked check passes as soon
as one of them is satisfied by one artifact. Advertising 3.11 and 3.12 while
shipping only a cp311 wheel therefore passed, leaving an advertised interpreter
with nothing to install. Coverage is now checked across the whole selected
artifact set, and validateTargetBindingsAgainstContractsV1 gives both the
per-artifact and the set-level check a caller in this slice.
Co-selectable payloads could collide. The parked tuple check gathers package
sets and exports and never gathers payloads at all, so two payloads reachable
in one support tuple could share a logical path or own overlapping install
destinations and still pass. Design rule 10 makes both semantic keys and allows
a shared unowned parent while forbidding overlapping owned trees, which is what
recordPathOverlapsV1 implements.
Probe identity is the sixth semantic key in that same rule and was likewise
unenforced. Identical probes deduplicate; the same executable invoked with
different arguments is now a conflict rather than two probes.
The composition validators have no production caller yet. PTD-06 owns the
release-graph walker that calls them, so they are exercised directly by tests
here rather than through a graph.
One observation recorded rather than fixed: validRecordValuesV1 is not
reference-closed, because the sample target names an unconditional payload the
shared set does not contain. Nothing before this slice resolved references, so
nothing noticed. The composition test helper supplies the missing payload
instead of changing a fixture four approved slices depend on.