Repository navigation
Conversation
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
github-actions
Bot
added
the
needs-review:MEDIUM
dinakarjs
changed the title
noah-ing
left a comment
noah-ing
left a comment
There was a problem hiding this comment.
Reviewed exact head cbd5e4f40e8ef85b9fdbb24e8b2a04acdc9bd625. The 41 harness tests pass, and I ran paired positive controls plus randomized scenario/context/time checks. Two negative controls still allow a historical synthetic result to be reported as applicable without a corresponding fresh run. The scope is local, caller-trusted evidence; these are correctness issues in the stated reappraisal behavior, not claims about authenticated production evidence. Please bind the retained result to the scenario it actually evaluated and to its original run/observation time, with regressions for both cases.
| _check_time(captured_at) | ||
| if not isinstance(context, EvaluationContext): | ||
| raise ValueError("explicit evaluation context required") | ||
| if (scenario.scenario_id, scenario.transaction_id, scenario.claim) != ( |
There was a problem hiding this comment.
[P2] Bind the result to the full scenario evaluated by the runner. This check compares only ID, transaction ID and claim. I ran a passing result from the baseline with a same-ID scenario whose workload input changed; a fresh runner call refused the changed scenario, but capture(changed_scenario, old_result, ...) succeeded and appraise returned applicable. The current hash then records the supplied scenario rather than the one the result came from. Please retain/check a scenario digest at run time, or explicitly make correct result-to-scenario pairing a caller obligation and limit the applicability claim accordingly.
| reasons.append("trust_changed") | ||
| if dict(snapshot.context.dependencies) != dict(current.dependencies): | ||
| reasons.append("dependencies_changed") | ||
| if evaluated_at < snapshot.captured_at: |
There was a problem hiding this comment.
[P2] Base evidence age on when the result was observed, separately from when a snapshot object was created. Re-capturing the same old RunResult with captured_at=1000 makes it applicable at evaluation time 1001 with a ten-second budget, while its original snapshot at time 100 is expired. An honest delayed capture can therefore refresh historical evidence without a new run. Please carry the original run/observation time into the snapshot and add a delayed-capture regression.
|
Addressed both reappraisal findings in The runner now records a digest of the full scenario and the original observation timestamp at run entry. Capture checks that digest, rejects missing/invalid observation metadata and refuses capture before observation. Appraisal measures evidence age from the retained observation time, so delayed capture cannot refresh it. Legacy results lacking these fields require a fresh run. The optional timestamp remains caller-trusted fixture input; hashes are not authentication. Regressions pair a passing original scenario with same-ID changed workload input, exercise mutable scenario input, contrast delayed old-result capture with a genuinely fresh run, and reject invalid/future/missing metadata. All 44 harness tests and 28 repository tests pass; Ruff and whitespace checks pass. Isolated removal of the full-scenario guard causes 2 failures; reverting age to capture time causes 1 failure. Published tree equals tested tree |
noah-ing
left a comment
noah-ing
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 0c7e518c16d52f6f4d6f1b06e1bf43164a7da427. Thank you for the focused fix: the two earlier reappraisal failures are resolved. All 44 harness tests pass; independent randomized controls rejected 900 same-ID scenario changes and exercised 1,000 observation/capture/expiry cases successfully.
One new contract failure blocks approval. A normal passing HarnessRunner().run(...).to_dict() now includes scenario_digest and observed_at, while the published result.schema.json sets additionalProperties: false and defines neither field. JSON Schema validation therefore rejects the runner's own result (the independent check also covered refused, unknown and incomplete results). Please update the schema with the intended types and legacy/required-field behavior, then add a regression that validates emitted results against it. The current CI jobs pass apart from the expected maintainer-approval gate, but they do not catch this producer/schema mismatch. This remains a local synthetic-evidence review, not a live or #199 acceptance decision.
| limitations: list[str] | ||
| package_versions: dict[str, str] = field(default_factory=dict) | ||
|
|
||
| scenario_digest: str | None = None |
There was a problem hiding this comment.
These fields are now included by RunResult.to_dict() on every run, but result.schema.json rejects both because it has additionalProperties: false and no matching properties. A passing positive-control result fails schema validation. Please add typed schema entries, choose whether legacy serialized results may omit them, and test the actual runner output against the schema.
There was a problem hiding this comment.
@noah-ing fixed in aff9e7eeeb0741ef898d2888645dae9a0c2bd360. The schema now defines the exact lowercase SHA-256 identity and non-negative integer observation time, requires the pair together when either is supplied, and continues accepting legacy serialized results with both absent. Legacy RunResult.to_dict() omits null defaults; EvidenceSnapshot.capture still rejects legacy results for reuse.
New tests validate actual emitted passed/refused/unknown/incomplete results, schema well-formedness, zero-time control, legacy serialization/refusal, partial metadata, invalid values and unknown-field rejection. All 50 harness tests pass normally and under -O; 28 repository tests pass; Ruff and whitespace pass. Restoring the old schema causes 5 assertion failures; removing metadata constraints causes 12. Published tree equals tested tree 7f548c6937c0a99f83b04f68dc783742acf08afc. The existing CI installs hash-pinned jsonschema and discovers the new test module; README local setup now uses that same dependency file.
All 11 current-head technical workflow runs, including CodeQL, now succeed. Please re-review this exact head. Formal re-review request returned connector HTTP 403, so this inline handoff records the request; independent maintainer acceptance remains separate.
noah-ing
left a comment
noah-ing
left a comment
There was a problem hiding this comment.
Re-reviewed exact head aff9e7eeeb0741ef898d2888645dae9a0c2bd360. The schema/producer mismatch from my last review is fixed: the published Draft 2020-12 schema now accepts actual emitted results, rejects partial or invalid metadata, and preserves the documented legacy pair behavior. I ran all 50 harness tests locally; an independent seeded sweep validated 2,000 runner outputs across all four terminal statuses. Technical CI is green apart from the separate maintainer-approval gate.
I am requesting changes on a separate confidentiality concern in the newly public result metadata. I filed a private pre-merge report with a synthetic reproduction through this repository's security-advisory channel, per SECURITY.md. Please have a named maintainer assess and resolve that report, including a regression for the documented public-result privacy boundary, before merging this head. I am keeping the reproduction out of the public PR discussion. This is a conditional issue in a caller-trusted synthetic harness; I have not found real protected-data exposure or a released affected version.
This review is limited to the local synthetic harness and is not the independent maintainer approval or full #199 acceptance.
|
@carloshvp @imran-siddique — #334 is now draft. Please coordinate assessment of Noah’s private report and provide access or an approved private repair route. An unpublished candidate passes 53 harness tests and 28 repository tests, but has not been checked against the confidential cases. The review hold remains pending that verification and independent acceptance. |
|
@dinakarjs added you to GHSA-gqxr-h639-qxch, the advisory for noah-ing's privacy finding on this PR, and it now has a private fork. Please put your candidate fix and a regression for the public-result privacy boundary there. This stays a draft until the fix lands. |
|
Self-review against the saved contribution guidelines (R34), 21 October 2026, public head Record this as a contribution-policy action for the coordinated revision: certify our own commits, preserve and verify the intended content/tree, and refresh exact-head checks and independent review after any history rewrite. I have left this public branch unchanged while the existing private assessment and draft hold remain in force. This administrative finding does not resolve or disclose the private report, and self-review supplies no independent approval. |
Signed-off-by: Srinivasa Consultants <78673153+dinakarjs@users.noreply.github.com>
… inputs Signed-off-by: Srinivasa Consultants <78673153+dinakarjs@users.noreply.github.com>
Signed-off-by: Srinivasa Dinakar <78673153+dinakarjs@users.noreply.github.com>
Validate emitted terminal results and preserve metadata-free legacy serialization. Require paired typed metadata when present, while refusing legacy reappraisal. Signed-off-by: Srinivasa Consultants <78673153+dinakarjs@users.noreply.github.com>
Retain private scenario binding and observation age while using explicit public projections. Add candidate-matching, status, future-field and mutation-isolation controls. Prepared with OpenAI Codex. Signed-off-by: Srinivasa Consultants <78673153+dinakarjs@users.noreply.github.com>
aff9e7e to
16c2553
Compare
|
The repair is now on this branch at |
|
@noah-ing @carloshvp requesting re-review at repaired head 16c2553, following the 23 October instruction to use this existing branch. The current public projection excludes the private metadata; the description names the normal/-O suites and failing-old-serializer controls. The older blocking review predates this head and needs independent disposition. Reviewer-assignment API returned 403; this is a request in the conversation, not a successful formal assignment. Draft retained. Hosted link/example failures are separate inherited Nobulex/OpenSSL blockers; no successful end-to-end, hardware or full #199 acceptance is claimed. |
noah-ing
left a comment
noah-ing
left a comment
There was a problem hiding this comment.
Reviewed exact head 16c25531a824c9300a1cbc2712dcdb4ded56f5db. The reappraisal, schema and public-result boundary concerns from my prior review are addressed for this caller-trusted synthetic harness. Independent public-projection, snapshot pairing and evidence-age controls passed across all four terminal statuses. The harness suite passed both normally and under -O (57/57 each); repository tests passed (28/28), and Ruff and whitespace checks are clean. This supersedes my earlier changes request, so I approve this bounded code repair. The PR remains a draft: hosted links, WCM example and maintainer gates, plus independent broader #199 acceptance, remain outstanding. This review makes no live, hardware or full #199 acceptance claim.
Preserve contribution history and evidence boundaries; validate the final merged tree. Signed-off-by: Srinivasa Dinakar Jainti <dinakarjs@gmail.com> Signed-off-by: Srinivasa Consultants <78673153+dinakarjs@users.noreply.github.com>
|
Current refresh 1d97461 now passes the returned hosted technical checks; its only returned failure is the normal maintainer gate. The synthetic harness source still contains the repaired result/scenario pairing, original evidence-age and public projection/schema behavior; all 57 tests pass normally and under -O. @noah-ing please re-review the synchronized head and disposition the three old inline threads that your 25 October bounded approval said were addressed. I have left your threads and independent approval decision to you rather than treating a branch refresh as reviewer acceptance. The existing 23 October public-branch repair route remains in force; no broader #199 or hardware acceptance is inferred. Prepared with OpenAI Codex. |
carloshvp
left a comment
carloshvp
left a comment
There was a problem hiding this comment.
Reviewed current head 1d97461de7fdd4e75c5d591761842b8da57bf9f0.
The repaired public projection is effective: private reuse metadata and future dataclass fields are excluded, emitted mutable values are copied, scenario/result pairing stays bound, and freshness uses the original observation time. I ran all 57 harness tests normally and under -O. Replacing only the serializer with the actual current-main method causes six of the eight privacy controls to fail by assertion, with no errors. This verifies the public repair on the existing branch; it does not call for a new private packet.
One scope correction remains before approval. The project-lead ruling places the reusable acceptance procedure in #199. This PR still adds its full duplicate as ACCEPTANCE-PROFILE.md and links it from the README. Remove that copy, link the canonical #199 procedure, and retain the utility-specific reproduction and trust-boundary documentation in the README. The runtime/evidence repair can stay in this PR. Hardware and the later independently operated acceptance remain separate, with #199 open. Sign-offs and diff whitespace checks pass.
Reviewed with OpenAI Codex.
Address current review scope and preserve unexecuted acceptance boundaries. Signed-off-by: Srinivasa Dinakar Jainti <dinakarjs@gmail.com> Signed-off-by: Srinivasa Consultants <78673153+dinakarjs@users.noreply.github.com>
|
@carloshvp Addressed your scope request in e858fe1: removed ACCEPTANCE-PROFILE.md and linked canonical #199 from the README, retaining utility reproduction and trust boundaries. Runtime source is unchanged. Root tests: 60 passed; harness: 57 normally and 57 under -O; 50 manifests/compatibility/generated checks pass. Published tree equals tested tree. Please re-review this scope correction; draft and independent acceptance boundaries remain. Prepared with OpenAI Codex. |
|
@carloshvp Re-review requested for e858fe1: your requested duplicate ACCEPTANCE-PROFILE.md removal and canonical #199 link are published, with runtime/regression bytes unchanged from the reviewed repair. All returned current-head technical workflows now succeed, including Validate integrations and documentation links. Please disposition your scope change request on this head. The recorded 57 normal/-O harness tests and 60 root tests are prior validation, not a fresh rerun in this follow-up. Draft remains pending your normal review; broader #199, live-host and hardware acceptance remain separate. No new private repair packet is proposed. Prepared with OpenAI Codex. |
|
@carloshvp — re-review follow-through updated 1 November 2026 at Your requested scope correction is published: the duplicate ACCEPTANCE-PROFILE.md is removed, the README links the canonical #199 procedure, and utility-specific reproduction/trust-boundary guidance remains. All returned current-head technical workflows pass. Please disposition the scope change request and state whether this bounded utility can leave draft, or identify the exact remaining author correction. Broader #199 acceptance, independently operated observations and hardware requirements remain separate. Prior local evidence is in the description; this updated request claims no new local rerun, independent approval or live acceptance. Prepared with OpenAI Codex. |
noah-ing
left a comment
noah-ing
left a comment
There was a problem hiding this comment.
Reviewed current head e858fe1d6b08363baf4eb976a0961d2585ac13fc. Compared with my earlier approved head 16c25531a824c9300a1cbc2712dcdb4ded56f5db, the harness runtime, tests and schemas are byte-identical; the harness changes are removal of the duplicate acceptance profile and a README link to the canonical #199 procedure. I verified those documentation changes and reran all 57 harness tests normally and under -O, plus all 60 repository tests, on this exact head. The returned hosted technical checks pass; the maintainer gate remains outstanding.
My three older inline findings on scenario binding, original observation age and public result schema are addressed in the current source and need no further code change. GitHub did not permit my account to toggle those thread states, so they remain visually open. I approve this bounded, caller-trusted reappraisal utility. The PR remains a draft pending Carlos's scope disposition and normal maintainer review; this approval does not establish the broader live or hardware acceptance tracked in #199.
A previously passing synthetic result can outlive its scenario, policy, trust inputs, dependencies or evidence-age budget. This adds a caller-trusted offline reappraisal utility beside the #199 harness: unchanged current inputs remain applicable; changed inputs, expired evidence and non-passing historical outcomes require reassessment with explicit reasons.
The runner retains the full scenario digest and original observation time privately. Public result and observation serialization use explicit field lists and copied mutable values; the public schema rejects private reuse metadata. EvidenceSnapshot keeps the binding and original observation time for pairing and expiry checks. Snapshot content and digests remain private local state.
The README links the canonical acceptance procedure in #199. The duplicate ACCEPTANCE-PROFILE.md is removed in response to Carlos's current-head scope review; utility-specific reproduction and trust-boundary documentation remain.
Validation of tree
2e702984c6572b24a2c8bca165033d5446666636, published ase858fe1d6b08363baf4eb976a0961d2585ac13fc:python -m pytest tests -q: 60 passed.python -m unittest discover -s tests -q: 57 passed, and 57 under-O.This utility does not authenticate snapshots or time, discover dependencies, persist revocations, dispatch work or authorize execution. Public field values remain caller-controlled. No hardware/live-host/independent-custody or #199 completion claim. Hardware work remains parked under WCM #200. Current-head hosted checks and independent review remain separate; draft retained.
Refs #199. Prepared with OpenAI Codex; DCO sign-offs retained; no self-approval or merge.