feat: preserve question dates and disclose CLI execution provenance#2
feat: preserve question dates and disclose CLI execution provenance#2100yenadmin wants to merge 3 commits into
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Adds the hermes-lcm TS provider (src/providers/hermes-lcm/*) and its Python bridge process, wired into the provider registry, config, and answer/judge CLI paths. This folds the previously-external V1 validation overlay (patch sha256 a38843cc..., decided under Fable program v2 / issue #131 on 100yenadmin/hermes-lcm) directly into this harness PR so the PR is self-contained for reproducing the 441/500 result. Per the honest-provenance rule, that result's attribution is harness + this overlay + product PR #423 together, not the harness alone. The M454 evidence-cards exclusion boundary is unchanged; that remains a separate V1-L1 candidate outside this PR's scope.
evaOS review status: completedPR: #2 - feat: preserve question dates and disclose CLI execution provenance evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #2 Review URL: #2 (review) |
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #2 - feat: preserve question dates and disclose CLI execution provenance
Head: c9fde575f2189acc5e90128c204b47675492d4c9 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
src/judges/cli.ts |
added | +41/-0 | Runtime code | Moderate: runtime path |
src/orchestrator/checkpoint.ts |
modified | +32/-16 | Runtime code | Moderate: runtime path |
src/orchestrator/index.ts |
modified | +41/-8 | Runtime code | Moderate: validated P3 finding |
src/orchestrator/phases/answer.ts |
modified | +78/-13 | Runtime code | Moderate: validated P2 finding |
src/orchestrator/phases/evaluate.ts |
modified | +58/-15 | Runtime code | Moderate: runtime path |
src/orchestrator/phases/report.ts |
modified | +41/-2 | Runtime code | Moderate: runtime path |
src/orchestrator/question-metadata.ts |
added | +35/-0 | Runtime code | Moderate: runtime path |
src/orchestrator/v1-release.test.ts |
added | +247/-0 | Test coverage | Elevated: large change |
src/providers/hermes-lcm/bridge/hermes_lcm_bridge.py |
added | +1120/-0 | Runtime code | Moderate: validated P2 finding |
src/providers/hermes-lcm/index.ts |
added | +287/-0 | Runtime code | Elevated: large change |
src/providers/hermes-lcm/ordinary-contract.test.ts |
added | +50/-0 | Test coverage | Moderate: runtime path |
src/providers/hermes-lcm/prompts.ts |
added | +27/-0 | Runtime code | Moderate: runtime path |
src/providers/index.ts |
modified | +11/-1 | Runtime code | Moderate: runtime path |
src/types/checkpoint.ts |
modified | +35/-0 | Runtime code | Moderate: runtime path |
src/types/judge.ts |
modified | +1/-0 | Runtime code | Moderate: runtime path |
src/types/provider.ts |
modified | +8/-1 | Runtime code | Moderate: runtime path |
src/types/unified.ts |
modified | +8/-0 | Runtime code | Moderate: runtime path |
src/utils/cli-llm.test.ts |
added | +229/-0 | Test coverage | Elevated: large change |
src/utils/cli-llm.ts |
added | +819/-0 | Runtime code | Elevated: large change |
src/utils/config.ts |
modified | +2/-0 | Configuration | Moderate: runtime path |
Review Signal
Validated inline findings: 3 (P0: 0, P1: 0, P2: 2, P3: 1).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- Auth: 1
- Runtime correctness: 2
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
Proof status: sufficient - PR metadata mentions acceptable proof for each required validation recommendation.
Profile validation hints: Prefer correctness, security, data-loss, release, and regression findings over style-only feedback.
Profile proof expectations: Look for focused validation, rollback notes, and evidence appropriate to the changed surface.
Related Context
Related issues/PRs: #423.
Suggested labels: tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: none
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
| # host-side evidence validation. These values already belong to | ||
| # the bounded lcm_recall response; the bridge never reopens the | ||
| # stores to enrich them. | ||
| metadata = _metadata_for_recall_hit(hit, dates) |
There was a problem hiding this comment.
P2: In-memory recency order resets on bridge eviction/restart, corrupting lcm_recall prior
Bridge._order is a plain in-memory dict that drives the per-session created_at used by lcm_recall's recency prior: order = self._order.get(container_tag, 0) + 1; ... created_at=float(order) (ingest, ~line 410-413). It is never persisted to the LCM store or a sidecar, and it is reset to {} whenever the bridge process for that container dies and is re-spawned. In src/providers/hermes-lcm/index.ts, getHandle() evicts LRU handles once MAX_BRIDGES is reached (line 219-225) and re-spawns a fresh process on next access; the same happens after any crash or process kill. On the next ingest the order counter restarts at 1, so a newly-ingested 20th session is stamped created_at=1.0 colliding with the first session, while the SummaryDAG nodes already on disk keep their original floats. This silently re-orders the recency prior and can change which evidence session lcm_recall surfaces, producing non-deterministic retrieval for the same already-ingested data. Persist _order (e.g. alongside the .dates.json sidecar or by reading max(created_at) from the DAG on first ingest per container) so the prior survives process recycling.
Category: Runtime correctness
Why this matters: Retrieval determinism is the whole point of the benchmark; a recency prior that flips depending on whether the bridge was evicted between ingest and search (or between two ingests) makes results unrepeatable and can hide real regressions.
| basePromptTokens, | ||
| contextTokens, | ||
| llmCall, | ||
| llmCalls: llmCall ? [...priorLlmCalls, llmCall] : priorLlmCalls, |
There was a problem hiding this comment.
P2: Failed retries inflate the per-question llmCalls ledger without distinguishing completed vs failed attempts
On the success path the phase records llmCalls: llmCall ? [...priorLlmCalls, llmCall] : priorLlmCalls (line 191). When the CLI path is used with retry: false this is a single call, but cliComplete still aggregates usage across attempts and the appended telemetry represents the whole call. The concern is narrower but real on the failure path (line 209): llmCall ||= cliCallTelemetryFromError(e) only attaches telemetry when the error is a CliCallError. A cliComplete call that fails on the FIRST attempt with retry:false throws a CliCallError (good), but any non-CLI failure (e.g. prompt-build throw before cliComplete is awaited, or an upstream error) leaves llmCall undefined, so llmCalls is set to priorLlmCalls and the failed attempt is invisible to the ledger. summarizeCliLedger/callLedgerComplete (cli-llm.ts:130) then report the ledger as complete for a phase that actually failed mid-call. Confirm whether failed-but-started attempts should be recorded as a distinct status telemetry entry so callLedgerComplete cannot read 'true' over a silent dropped call.
Category: Runtime correctness
Why this matters: The CLI ledger/provenance is the disclosure mechanism this PR adds; a 'complete' ledger over a dropped attempt misrepresents which model actually produced (or failed to produce) an answer.
| role: "answerer" | "judge" | ||
| ): LlmExecutionProvenance { | ||
| const cli = cliLlmProvenance(role) | ||
| if (cli) return { ...cli, configuredModel, tokenizerModel: "gpt-4o" } |
There was a problem hiding this comment.
P3: Hardcoded tokenizerModel=gpt-4o for every CLI backend
resolveExecutionProvenance returns { ...cli, configuredModel, tokenizerModel: "gpt-4o" } for any CLI backend (line 90), including claude/non-OpenAI models. Token counts in the answer phase are still computed via countTokens(prompt, modelConfig) where modelConfig is getModelConfig(DEFAULT_ANSWERING_MODEL) (gpt-4o) on the CLI path (answer.ts:120-121), so the value is internally consistent today, but the provenance field asserts the tokenizer model is gpt-4o regardless of the actual backend. Either derive tokenizerModel from the resolved modelConfig actually used for counting, or document that gpt-4o tokenization is applied to all CLI backends by design, so downstream readers don't infer Anthropic-native token counts.
Category: Auth
Why this matters: Provenance fields are meant to disclose exactly how a run was executed; an unconditional tokenizer label can mislead analysis comparing token economics across codex vs claude backends.
![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: c9fde575f2
ℹ️ 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".
| answererProvenance: overrides?.answeringModel ? undefined : source.answererProvenance, | ||
| judgeProvenance: overrides?.judge ? undefined : source.judgeProvenance, |
There was a problem hiding this comment.
When the server creates an advanced copy it always passes answeringModel || sourceCheckpoint.answeringModel and a required judgeModel, so these truthy checks clear both provenance fields even when the copied run starts from evaluate or report and the corresponding completed answer/evaluation phases are kept. That produces reports with retained CLI answers/judgments but no actual execution identity, which regresses the provenance this change is trying to preserve; only clear provenance when that phase is actually reset or the model really changes.
Useful? React with 👍 / 👎.
| order = self._order.get(container_tag, 0) + 1 | ||
| self._order[container_tag] = order |
There was a problem hiding this comment.
If a Hermes ingest is interrupted after some sessions and later resumed, the TypeScript provider starts a fresh bridge process, so this in-memory _order map is empty even though the container DB already has earlier summaries. Resumed sessions then start again at created_at=1, which breaks the monotonic recency prior used by the bridge/search path and can corrupt temporal or knowledge-update rankings after a normal checkpoint resume; derive the next order from persisted container state before appending.
Useful? React with 👍 / 👎.
| if (phases.includes("answer")) { | ||
| checkpoint.answererProvenance = resolveExecutionProvenance(answeringModel, "answerer") | ||
| } | ||
| if (phases.includes("evaluate")) { | ||
| checkpoint.judgeProvenance = resolveExecutionProvenance(judgeModel, "judge") |
There was a problem hiding this comment.
On a normal resume the default phase list includes answer and evaluate, so this overwrites the checkpoint provenance before those phases discover that all questions are already completed. For AI SDK runs there is no per-call ledger to reconcile, so simply regenerating a report with a different -m/-j value or CLI environment can record the current labels as the actual answerer/judge even though the retained answers and judgments were produced earlier; only refresh these fields when the phase is actually reset or has pending work.
Useful? React with 👍 / 👎.
| basePromptTokens, | ||
| contextTokens, | ||
| llmCall, | ||
| llmCalls: llmCall ? [...priorLlmCalls, llmCall] : priorLlmCalls, |
There was a problem hiding this comment.
If a question has a failed CLI answer attempt and the run is resumed with HERMES_MB_LLM_CLI unset, the successful AI SDK answer leaves llmCall undefined, so this carries the old failed CLI ledger into the completed phase. The report reconciles provenance from any stored CLI call and will attribute that AI SDK answer to the previous CLI identity (and mark usage incomplete); clear old CLI calls when a non-CLI retry succeeds, or only preserve them when appending a new CLI call.
Useful? React with 👍 / 👎.
| text = await cliComplete(prompt, { | ||
| role: "answerer", | ||
| retry: false, |
There was a problem hiding this comment.
When HERMES_MB_LLM_CLI is set, the normal -m/--answering-model value is still stored in checkpoint.answeringModel, but this CLI path never passes it to cliComplete. Unless a separate HERMES_MB_CODEX_MODEL/HERMES_MB_CLAUDE_MODEL env var is also set, the benchmark runs the subscription CLI's default unpinned model while the run arguments show the requested model; pass the checkpoint model as the CLI model override or reject the unsupported combination.
Useful? React with 👍 / 👎.
| const handle = this.spawnHandle(tag) | ||
| this.handles.set(tag, handle) |
There was a problem hiding this comment.
Each new container tag stores a long-lived Python bridge process in this.handles, but the orchestrator creates a provider for a run and never calls any teardown after search/report. Successful CLI runs can stay alive because the child stdio handles are still open, and the server will accumulate bridge processes across runs until eviction happens within one provider instance; add a provider close/dispose path or close all handles once the run is finished.
Useful? React with 👍 / 👎.
1 participant
Summary
questionDatethrough checkpoint initialization, legacycheckpoint backfill, answer prompts, copied runs, and resumed runs.
CLI transport, version, explicit model pin, reasoning effort, provider, and
service tier.
retry count, timing, and event-stream digest; prompts, answers, and raw event
payloads are never retained.
skills, MCP servers, repository rules, and arbitrary host environment values.
run does not break answer/resume.
partial copy cannot be reported as a completed answer run.
Why
Benchmark output must distinguish the model configured in run arguments from
the model and transport that actually answered or judged. Temporal questions
also need their declared question date to survive checkpoint and copy/resume
boundaries.
Compatibility and safety
HERMES_MB_LLM_CLIis unset.an unpinned default when no model is supplied.
locale, and CLI-auth paths but not provider keys, benchmark variables,
database URLs, or arbitrary host secrets.
configuration, and no tools; authentication is injected ephemerally from the
existing Keychain entry when available.
overwritten with current environment labels.
Validation
The repository-wide
tsc --noEmitcurrently stops on the same twopre-existing type errors in unchanged
src/providers/cortex/index.ts. Therepository-wide format script also reports existing formatting drift, now in a
different set of nine files (unchanged count; pre-existing drift, not
introduced by this PR). The candidate surface type-checks, all repository
tests pass, the application bundles, and no remote CI exists in the current
upstream tree.
Validation overlay now included
The
src/providers/hermes-lcm/*provider (TypeScript provider plus the Pythonbridge process) is included directly in this PR, not applied separately. This
PR is therefore self-contained for reproducing the reported 441/500
score-preservation result. Per the honest-provenance rule, that result is
attributed to the harness (this PR) together with the overlay and product PR
stephenschoettler/hermes-lcm#423
combined — not to the harness alone.
Deliberate scope boundary
This PR does not include M454 evidence-card answer presentation, adaptive or
agentic retrieval, selectors/controllers, product-specific evidence compilers,
or any benchmark score claim. M454 was a separate host-presentation experiment
that improved its evidence card by
+4(14/20versus10/20) but missed itspredeclared
+5gate.