Repository navigation
Feat/learner ux v6.1 - #2
Conversation
|
Important Review skippedToo many files! This PR contains 235 files, which is 135 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (235)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 3c50ebf1af
ℹ️ 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".
| "--symlink", | ||
| "usr/lib", | ||
| "/lib64", |
There was a problem hiding this comment.
In the Debian Trixie amd64 runner image, dynamically linked executables such as /usr/bin/bash use /lib64/ld-linux-x86-64.so.2, while the merged-/usr compatibility target is usr/lib64, not usr/lib. This sandbox therefore cannot launch the Bash-based isolation probe (and may fail any other dynamically linked command using that interpreter), causing the mandatory startup probe to fail and preventing every hosted runner instance from becoming ready.
Useful? React with 👍 / 👎.
| await connection.close(); | ||
| await launch.dispose?.(); |
There was a problem hiding this comment.
If initialization fails before revokeCredentials() and the child does not emit close within the new five-second shutdown bound, connection.close() rejects and JavaScript skips the following launch.dispose(). For the hosted boundary this leaves the staged auth file containing the credential on disk, so disposal must run in a nested finally even when process termination fails.
AGENTS.md reference: AGENTS.md:L472-L474
Useful? React with 👍 / 👎.
| await containedNewOutputFile( | ||
| this.options.repositoryRoot, | ||
| outputFile, | ||
| "CloakBrowser raw run evidence", | ||
| ); | ||
| await writeFile(outputFile, `${JSON.stringify(report, null, 2)}\n`, { | ||
| encoding: "utf8", | ||
| flag: "wx", | ||
| mode: 0o600, |
There was a problem hiding this comment.
With the default configuration, every run targets the fixed apps/web/test-results/runtime/m8-local/evidence/cloakbrowser-raw-run.json, which is outside Playwright's cleared outputDir. After the first ordinary E2E run leaves that file behind, containedNewOutputFile() rejects it on the next run and the catch returns { status: "failed" }, so an otherwise passing suite deterministically fails on every rerun unless someone manually deletes evidence or chooses a new runtime root.
Useful? React with 👍 / 👎.
| await rename(stagingRoot, expectedRoot); | ||
| const publishedReservation = await containedNewOutputFile( | ||
| input.repositoryRoot, | ||
| resolve(expectedRoot, ".publication-reservation.json"), | ||
| "published CloakBrowser qualification reservation", | ||
| ); | ||
| await rename(reservation, publishedReservation); |
There was a problem hiding this comment.
If the process crashes or the second rename fails after stagingRoot has been moved to expectedRoot, the published directory exists without .publication-reservation.json. Every retry then fails the initial assertPathAbsent(expectedRoot) check, leaving that deployment receipt permanently wedged with incomplete qualification evidence; move the reservation into the staging tree before the single publication rename or provide an explicit recovery path.
Useful? React with 👍 / 👎.
| const authorities = [ | ||
| { | ||
| name: "GPT-5.6", | ||
| responsibility: "Proposes a bounded belief frame", |
There was a problem hiding this comment.
When OPENAI_MODEL overrides the default or OPENAI_BASE_URL selects a compatible provider, the new landing strip still tells every learner that "GPT-5.6" proposes the belief frame. Since the Worker accepts and forwards those configurations, this creates a false public authority claim; use a provider-neutral role label or expose only a verified server-side model identity instead of hardcoding the default.
AGENTS.md reference: AGENTS.md:L426-L430
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c491afe772
ℹ️ 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".
| if (!Number.isFinite(timestamp)) return true; | ||
| return ( | ||
| now.getTime() >= | ||
| Math.min(timestamp + MUTATION_CLAIM_TTL_MS, jobDeadline(job)) |
There was a problem hiding this comment.
When reading or storing a runner output takes more than 60 seconds, this marks the still-active claim as expired even if the job deadline is several minutes away. A callback, cancellation, or timeout can then remove the claim and terminalize the job while the original PUT continues, allowing runner-output bytes to change after verification or cancellation; retain or renew the claim for the active request, and only recover abandoned claims at the bounded job deadline as intended.
AGENTS.md reference: AGENTS.md:L531-L534
Useful? React with 👍 / 👎.
| : { beliefSpec: session.beliefSpec }), | ||
| ...(session.learningDirector === undefined | ||
| ? {} | ||
| : { learningDirector: session.learningDirector }), |
There was a problem hiding this comment.
When OPENAI_BASE_URL points to a compatible provider whose Responses payload returns a provider- or deployment-specific model value, directorResult.provenance.modelId is persisted and this newly exposed learningDirector object sends it to browser state on every session response. This is a separate exposure from the hardcoded display-copy issue already reported; project provider-neutral provenance or keep the raw model identity server-side.
AGENTS.md reference: AGENTS.md:L426-L432
Useful? React with 👍 / 👎.
| return context.json(jsonSuccess(statePayload(proposed))); | ||
| if (result.beliefSpec.supportState === "SUPPORTED") { | ||
| try { | ||
| const directorResult = await createLiveLearningDirectorFromEnv( |
There was a problem hiding this comment.
For a supported live claim, this starts the Learning Director synchronously after the Belief Analyst has already completed, and the director may make three sequential Responses calls, each with a default 180-second timeout. The browser client aborts every ordinary API request after at most 210 seconds, so a valid tool-using analysis can outlive the caller, leaving a persisted Belief Spec or Director decision that the UI reports as failed; enforce one shared deadline below the client limit or move the director to a separately resumable request.
AGENTS.md reference: AGENTS.md:L430-L432
Useful? React with 👍 / 👎.
| scientificEngineSnapshotHash: | ||
| scientificEngineSnapshotValue.authorityHash, |
There was a problem hiding this comment.
After any accepted scientific-engine upgrade changes snapshot-hash.json, this compares every stored schema-v2 Proof Bundle with the new runtime snapshot rather than the snapshot recorded when the evidence was issued. Because statePayload() now runs this check for every session response, all completed live investigations from the prior engine version begin returning PROOF_BUNDLE_INVALID even though their content hash and provenance remain valid; verify historical bundles against their recorded or issuance-time snapshot instead of the current deployment.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
| const pattern = inventory.replacementEntries | ||
| .map(({ normalizedName }) => escapedRegex(normalizedName)) | ||
| .join("|"); | ||
| safe = safe.replace(new RegExp(pattern, "giu"), (match) => { |
There was a problem hiding this comment.
When a supported notebook declares a short sensitive field such as ID, the case-insensitive alternation matches those letters inside unrelated words—for example, valid is rewritten as val[REDACTED_SENSITIVE_FIELD_…]. Because this sanitizer is applied to the learner claim, evidence excerpts, symbols, metrics, and schema types, ordinary packets can become badly corrupted before either reasoning request; match complete identifier tokens or syntax-aware field references rather than arbitrary substrings.
AGENTS.md reference: AGENTS.md:L434-L437
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f32b05c47
ℹ️ 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".
| ...(session.proofBundle.integrity.mode === "hmac-signed" && | ||
| usableSigningKey !== undefined | ||
| ? { signingKey: usableSigningKey } |
There was a problem hiding this comment.
When COUNTERLAB_SIGNING_KEY is rotated after an HMAC-signed Proof Bundle was issued, this always validates the historical signature with the current key. Because statePayload() now runs this check for every session response, completed investigations signed under the previous key become inaccessible and Proof Bundle export returns PROOF_BUNDLE_INVALID; retain issuance-key provenance and the corresponding verification key instead of assuming the active key signed every stored bundle.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
| if (!callbackClaimExpired) { | ||
| throw new RunnerCallbackStateError( | ||
| `Runner callback ${parsed.idempotencyKey} is already being processed`, | ||
| ); |
There was a problem hiding this comment.
If a Worker invocation terminates after persisting this callback claim but before recordCallback() or the error handler releases it, every matching retry is rejected here for 60 seconds. The hosted runner's HttpRunnerControlPlane.callback() makes only three attempts with default delays of 250 and 500 ms (and at most 15 seconds even with its maximum configured delay), so it exhausts recovery before the claim can be reclaimed and the valid terminal callback is never projected; make retries span the claim-expiry window or allow a safely matching retry to take over.
AGENTS.md reference: AGENTS.md:L531-L533
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6608ad2ab9
ℹ️ 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".
| kind: "decrease", | ||
| symbol: directionSymbols.decrease, | ||
| label: "Lower on unseen customers", | ||
| accessibleLabel: | ||
| "Downward change: the verified result is lower on unseen customers.", |
There was a problem hiding this comment.
For any leakage comparison whose labels mention familiar rows and unseen/new customers, this hardcodes a decrease without inspecting the signed metrics. The fixed outcome classifier explicitly accepts gaps up to 0.03 as the current-hypothesis pattern and gaps between 0.03 and 0.10 as inconclusive, including equal values or a group-holdout accuracy up to 0.02 higher (packages/concept-registry/src/index.ts lines 550-567); those valid live results would therefore display a false down arrow and “Lower on unseen customers.” Require the caller to supply a direction derived from the verified payload, or compute it from numeric values.
AGENTS.md reference: AGENTS.md:L661-L665
Useful? React with 👍 / 👎.
| reasoningDiffHash, | ||
| ] = await Promise.all([ | ||
| canonicalSha256(session.beliefSpec), | ||
| canonicalSha256(session.prediction), |
There was a problem hiding this comment.
Every genuine native Proof Capsule session records reasoningDiffV2.authority.predictionHash as prediction.immutableHash (packages/session-core/src/service.ts line 1283), but this hashes the entire Prediction object instead. Since immutableHash was computed from the Prediction fields before that property was added (apps/web/worker/api.ts lines 5622-5636), the two digests cannot match; the client therefore rejects the successful capsule-issuance response and every subsequent session refresh as NATIVE_PROOF_LINEAGE_INVALID. Compare the authority to session.prediction.immutableHash instead.
AGENTS.md reference: AGENTS.md:L967-L968
Useful? React with 👍 / 👎.
No description provided.