Repository navigation
Conversation
Add a versioned, bounded envelope for the structured outcome of a pipeline stage attempt, with producer provenance (AgentRun or operator), typed validation and a pure delivery assessment that classifies a delivery as accepted, duplicate, stale or conflict without granting retry authority. The contract is provisional run evidence: no persistence, no wiring into the stage completion path, and no Artifact, review or approval semantics. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018V8SDpmPqvp85atzRorQoT
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. |
![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: 28e0de53b0
ℹ️ 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".
Authenticate the producer: the assessment now compares the payload's claimed producer with the deliverer established by the Runtime, so a superseded AgentRun can no longer pass as an operator. A redelivered recorded result is a duplicate even after its run is unbound. Bound the work before normalizing untrusted data (depth, node budget and cheap length checks), reject credential-looking text and any data key that contains a sensitive name, forbid control and bidi/line-separator characters and line breaks outside the summary, and read each input field once. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018V8SDpmPqvp85atzRorQoT
Stop rejecting ordinary prose: credential detection now looks for token-shaped material (long or digit-bearing bearer values, base64-shaped basic credentials, provider token prefixes, key=value with letters and digits) instead of words such as "basic" or "token:". Token-usage metering keys such as totalTokens are accepted; a bare token key is still rejected. Apply the character and credential checks to data keys, reject zero-width marks, byte-order marks and lone surrogates, bound the cumulative size of data strings and keys before normalizing, and normalize a plain snapshot taken in the guard so a changing object cannot bypass the checks. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018V8SDpmPqvp85atzRorQoT
…tors A throwing accessor on the envelope, producer or error object escaped as an arbitrary Error and could run caller-controlled code. Fields are now read once through own data descriptors into a plain snapshot, an accessor is refused without being invoked, and hostile input that throws while it is read (throwing or revoked Proxy) surfaces as the typed validation error. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018V8SDpmPqvp85atzRorQoT
… bytes Snapshotting turned a Map, Set, Date, RegExp, typed array or class instance in data into a plain object of its own fields, so different values could become canonically equal. Only plain and null-prototype objects are accepted now. Data keys are measured in UTF-8 bytes as documented, and the key checks assert the rejection reason. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018V8SDpmPqvp85atzRorQoT
The data snapshot silently discarded extra array properties, symbol keys and non-enumerable properties, which canonical JSON would have rejected. They are refused now. The remaining realm and reserved-key limits are documented. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018V8SDpmPqvp85atzRorQoT
…perator pre-emption Check length, line breaks and characters on the raw string before trimming, reject every Unicode format, private-use and surrogate code point and a lone CR, and pass through only the parser's own rejections so an error thrown by a hostile trap is replaced. Refuse non-enumerable array indices, non-plain envelope records and arrays, and bound object width. Anchor credential patterns with lookarounds, cover JWT, Stripe, GitLab and URL userinfo, keep provider usage metering keys. The delivery assessment revalidates the recorded result, reads its context once and marks an operator delivery that pre-empts a bound AgentRun. Claude-Session: https://claude.ai/code/session_01EVFrKKyq46D9yGUtsWrPNY
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Both P2 findings are fixed and covered by regression tests: accessor/descriptor refusal without invoking getters ( |
avalla
left a comment
avalla
left a comment
There was a problem hiding this comment.
Review indipendente su dadf87b (ChatGPT, non Codex CLI/cloud). Ho esaminato i 4 file della PR, il comportamento del parser e la classificazione delle delivery, il feedback Codex precedente e i check GitHub. 1 finding P2 nella whitelist dei campi sensibili (commento inline). Non ho identificato altri P1/P2 confermati. Prima di dichiarare READY FOR MERGE: correggere il P2 e aggiungere test di regressione; verificare macOS validate (in progress all'ultima lettura); aggiornare il branch rispetto a main (79 commit più recente alla verifica) e rieseguire i gate sull'HEAD finale. L'autore dichiara bun run check verde (3567 passed, 239 skipped), ma non ho eseguito personalmente la suite locale. Questa review non è una review eseguita da Codex e non sostituisce automaticamente un eventuale gate formale che la richieda.
A bare tokens, tokensUsed or tokensCount key bypassed the sensitive-name check. Claude-Session: https://claude.ai/code/session_01EVFrKKyq46D9yGUtsWrPNY
|
Re the ChatGPT review on dadf87b: the P2 (optional prefix let a bare |
A nested accessor on deliveredBy could return different producers to the producer comparison and the stale check. The deliverer is now copied into a plain value before either runs. Adds the regression test and a test that pins the object width cap against the remaining node budget. Claude-Session: https://claude.ai/code/session_01Q21NUKiVwrCvpZ5YgzjM5W
Accept-at-limit and reject-at-limit-plus-one coverage for summary, error message, identifier, data key, depth and node count, so an off-by-one in any bound is caught. Test-only change. Claude-Session: https://claude.ai/code/session_01FKM37BV4LS8Rc8ny5GCwXy
…ined error A hostile trap that held an earlier parser rejection could rewrite its message and rethrow it; the WeakSet membership test then forwarded the mutated text. Rejections are now tracked with their reason in a WeakMap and a fresh, sanitized rejection is thrown from that private reason. Claude-Session: https://claude.ai/code/session_01FKM37BV4LS8Rc8ny5GCwXy
|
Update for head c88c3fd: added exact-boundary limit tests (all limits) and fixed a Codex P2: a retained parser rejection whose message a hostile trap mutated was forwarded; rejections are now rebuilt from a private reason (regression test added). Evidence: |
avalla
left a comment
avalla
left a comment
There was a problem hiding this comment.
Independent ChatGPT review — HEAD c88c3fd (static source review, not Codex CLI)
Technical verdict: no newly confirmed P1/P2 findings in the reviewed code. Reviewed the stage-result parser, delivery assessment, documentation and relevant regression tests. The parser snapshots untrusted own data properties, bounds data, rejects unsupported shapes, canonicalizes results and now reconstructs validation errors from privately retained reasons rather than rethrowing mutable error objects. The delivery assessment preserves producer-mismatch / duplicate / stale / conflict precedence and flags operator pre-emption.
Existing lower-severity limitations: the scanner intentionally does not guarantee exhaustive secret detection; malformed trusted delivery context is not revalidated; identifiers are trimmed before equality; performance follow-ups are tracked. These are not newly proven merge blockers for this provisional, non-wired contract.
Evidence: GitHub workflows on the published HEAD are green; I did not execute Bun or perform mutation tests in this review. The PR description's consolidation note still names an earlier HEAD and should be brought up to date (P3 documentation hygiene).
Gate: This is a separate static ChatGPT review; it does not attest that codex review --base origin/main ran successfully and must not be counted as that configured gate.
…exactly Close the remaining P3 findings of the stage result contract review: - The authenticated deliverer and the bound AgentRun id are validated with the producer rules, so a malformed context is a typed DomainValidationError instead of a silent producer_mismatch or stale decision. - Producer identifiers are compared exactly: padding is refused rather than trimmed, so " run-1 " and "run-1" can no longer be treated as one producer. - The URL-userinfo credential pattern matches "://" first and checks the scheme backwards, which removes its quadratic cost on long scheme-looking runs (about 41 ms to 2 ms at 16 KB). - The credential scanner now also refuses an empty URL user (redis://:pw@), the whsec_ prefix and a JWT glued to a word (token_eyJ...). Claude-Session: https://claude.ai/code/session_01FKM37BV4LS8Rc8ny5GCwXy
The deliverer was destructured before validation, so extra fields, inherited properties, a throwing getter or a revoked proxy bypassed the producer rules. Pass it to parseStageResultProducer as is: own data properties are read once, and anything else is a typed, sanitized DomainValidationError. Also pin the linear cost of the URL credential check with a scaling test, so the quadratic pattern cannot return unnoticed. Claude-Session: https://claude.ai/code/session_01FKM37BV4LS8Rc8ny5GCwXy
|
Hardening cycle closed on |
Summary
Adds a provisional, versioned contract for the structured outcome of a pipeline stage attempt, with producer provenance and a pure delivery assessment. It is run evidence only: no persistence and no wiring into the stage completion path.
Task
04478a78-d9b9-4409-a05b-ab05d54b9278— M11-1 — Definire il contratto provvisorio degli artifactScope
packages/domain/src/pipeline/stage-result.ts), application delivery assessment (accepted/duplicate/stale/conflict/producer_mismatch, plus apreemptsBoundRunflag), tests, documentation indocs/development/pipeline-enforcement.md.19328245-e21c-48ce-9732-13fd1a99ef4c("M11/M12 — Trasportare l'envelope stage result nel run result e nell'audit sanitizzato"), with verifiable acceptance criteria, depends on M12-1 (76202083-16b6-47e5-b5ed-710b516b42d6, whose normalized result is the carrier) and on M11-1. M12-1 itself stays port-only (no persistence, no audit wiring), so it does not own the transport.Design notes
producerin the payload is a claim; it must equal the deliverer the Runtime authenticated. An operator label is self-declared and is not authenticated against another same-UID process.canonicalStringify), and a recorded result is re-validated first. A different result for the same attempt is aconflictand is never overwritten; a result from an AgentRun no longer bound isstale. An accepted operator delivery while a run is bound setspreemptsBoundRun: a caller must record it through the existing attributed override path with a reason.summaryand data string values; Unicode format, private-use and surrogate code points are refused; only the parser's own rejections pass through the boundary, errors thrown by hostile traps are replaced; errors name the field only.inputTokens,reasoning_tokens,cache_read_input_tokens, ...); a baretokensis refused.Verification (HEAD
dc1618a)bun test tests/unit/stage-result-contract.test.tsbun run checkondc1618aorigin/main(0c324b9) intodc1618ain a temporary worktree,bun run checkgit diff --check origin/main...HEAD, Prettier, eslint, tscdc1618amacOS validatefailure infilesystem-connector.test.ts, a file this PR does not touch, passed on rerun)Review
Reviews are by subagents of the same model family as the author; they are not independent in the strong sense and none is a Codex review.
c988426): no P1; one P2 (operator pre-emption) addressed bypreemptsBoundRun; doc-accuracy P3s corrected.c988426): 1 P2 (invisible characters) and 12 P3; the in-scope ones are fixed in258796a. Not fixed, by decision: homoglyph/fullwidth data keys bypass the sensitive-name check becausenormalizeSensitiveFieldKey(shared with capability authorization) drops non-ASCII characters; this belongs in a separate change.c988426): about 190 own assertions against the final code, no defect.dadf87b..dc1618a(metering allowlist and its test): APPROVE, no P1/P2; mutation confirmed the new test kills the old regex.c988426..dadf87b(258796a,dadf87b: hostile input, Proxy/accessors, typed sanitized errors, credentials, hidden properties, Unicode, data limits, delivery classification): APPROVE, 0 P1, 0 P2, 4 P3; 17 of 18 mutations were caught by the tests; 113 tests pass. The 4 P3 are deferred, not blocking, and tracked in task3ec1344e-221e-4468-86f2-2a77428f86d7: (1)deliveredByis read more than once, so a nested accessor could flip the decision (assess-stage-result-delivery.ts:59-88); (2) JWT and URL-userinfo patterns are quadratic, ~38 ms at 16000 chars, bounded by the data budget (stage-result.ts:120,122); (3) credential scanner gaps (redis://:pass@host,token_eyJ...,whsec_) and false positivepwd=/home/user1; (4) the object width-cap line is not proven by a test (stage-result.ts:345-346).dc1618a:..c988426architecture, security and QA;c988426..dadf87banddadf87b..dc1618aincremental reviews (both APPROVE). Every range has a non-Codex review; no range has a Codex review after258796a.dadf87b: one P2 (baretokensbypass), fixed indc1618a, thread resolved.258796aanddadf87b.External review (Codex)
Required:
external_review.commandis configured in.task-delivery.yaml. Not satisfied on the final head. The last Codex review that completed ran on258796a. Codex CLI and the GitHub Codex connector both report that the usage limit is reached (available again on 2026-10-22), so no Codex review exists fordadf87bordc1618a. This is a failed gate, not a pass, and no other review stands in for it.Limitations
Merge
Do not merge without explicit authorization. READY FOR MERGE is not DONE: post-merge verification is still required.
https://claude.ai/code/session_01EVFrKKyq46D9yGUtsWrPNY
Hardening update (2026-10-19) - HEAD
7cf05ccAll P3 left by the earlier reviews were re-verified on the remote head and the confirmed in-scope ones fixed:
DomainValidationError, never a silent decision).://first and checks the scheme backwards: about 41 ms to under 2 ms at 16 KB (worst case of every pattern measured); a scaling test pins it. Equivalence with the old pattern checked on 300k random strings; the only difference is the intended empty user.redis://:pw@host,whsec_andtoken_eyJ...(credential exposure, P3).Evidence:
bun run checkexit 0 on7cf05cc(3597 passed, 239 skipped); GitHub CI 13/13 green; new tests fail on the previous code. Independent read-only review ofc88c3fd..fd0d5bd: 2 P3 (deliverer read outside the parser boundary; no test for the linear cost), both fixed in7cf05cc.Not in this PR, tracked in task
3ec1344e-221e-4468-86f2-2a77428f86d7: homoglyph/fullwidth data keys (the sharednormalizeSensitiveFieldKeyalso serves capability authorization, so a fix changes authorization behavior);pwd=/home/user1over-refusal (fail-closed, not an exposure); no roadmap entry for M11-1.External review (Codex): not satisfied on the final head. The Codex CLI and the GitHub connector are unavailable (usage limit); no other review stands in for it. The independent review below is by a subagent of the same model family, not Codex.