Repository navigation
Conversation
…d and why) Closes tester-army#939 Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
1 issue found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/e2e/src/run/steps.ts">
<violation number="1" location="packages/e2e/src/run/steps.ts:145">
P2: This exported type accepts outcome/reason combinations that the report schema rejects, and discriminating on `outcome` does not narrow `outcomeReason`. Model these fields as a discriminated union so SDK values match the schema.</violation>
</file>
Reply to a comment to ask cubic a question or push back. It learns from your replies.
Turn on auto-fix | Re-trigger cubic
| * nothing a replay could check to record in the entry's place), | ||
| * `unconfirmed` (no verification passed after the step). | ||
| */ | ||
| outcomeReason?: 'confirmed' | 'unchanged' | 'repaired' | 'failed-after-replay' | 'not-replaced' | 'unconfirmed'; |
There was a problem hiding this comment.
P2: This exported type accepts outcome/reason combinations that the report schema rejects, and discriminating on outcome does not narrow outcomeReason. Model these fields as a discriminated union so SDK values match the schema.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/e2e/src/run/steps.ts, line 145:
<comment>This exported type accepts outcome/reason combinations that the report schema rejects, and discriminating on `outcome` does not narrow `outcomeReason`. Model these fields as a discriminated union so SDK values match the schema.</comment>
<file context>
@@ -121,6 +121,28 @@ export interface StepCacheInfo {
+ * nothing a replay could check to record in the entry's place),
+ * `unconfirmed` (no verification passed after the step).
+ */
+ outcomeReason?: 'confirmed' | 'unchanged' | 'repaired' | 'failed-after-replay' | 'not-replaced' | 'unconfirmed';
replayedActions: number;
totalActions: number;
</file context>
There was a problem hiding this comment.
Leaving this as is for now, and leaving the thread open for a maintainer's call. Turning StepCacheInfo from an interface into a union would change a public type's shape: an interface X extends StepCacheInfo in user code stops compiling, which is a bigger contract change than this additive PR. The existing pairs on the same type are just as loose in TypeScript and are enforced only by the report schema: reason is optional on every mode, and derived is legal only with reason: 'gap'. outcome/outcomeReason follow that convention, and report-v1.schema.json rejects every invalid combination (pinned in schema-fixtures.test.ts). Happy to switch to a discriminated union if the maintainers prefer it.
FileCacheStore.delete rejects on failures other than an absent file, so neither the step nor attempt-end settlement reports an entry left on disk as evicted. Docs: qualify the unchanged file on a confirmed replay, the model-unavailable exception, and where agent.act() results stop short of the report. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks. This landed in a different shape in #869 as |
Closes #939.
What and why
StepCacheInfosays how a step read the cache (mode,reason) but not what the attempt then did to the recording. A self-healing CI loop needs that to know which tests to run a second pass for, and today it has to diff.e2e/cache/against git.outcome: 'written' | 'kept' | 'evicted'outcomeReason: kept isconfirmedorunchanged; evicted isrepaired,failed-after-replay,not-replacedorunconfirmed; absent onwritten.read-writemode when the attempt changed or confirmed an entry. A read-only cache, an interrupted attempt, a step with no verdict, or nothing to record and nothing stored leavesoutcomeabsent. No schema version bump.delete, a rejected delete, or a failed write reports no outcome.Each outcome comes from the place that already makes the decision:
packages/e2e/src/agent/step-cache.ts,evict(reason)sets the outcome on the step's cache info once the delete has settled.conclude()runs beforefinish()attaches the details, so these reach both the report andagent.act()'s result.flushStagedTraces(packages/e2e/src/cache/context.ts) now returns{ stepIndex, outcome, outcomeReason? }[], andpackages/e2e/src/run/execute.tsmerges them intorecord.steps[stepIndex].cacheright after the flush.agent.act()returns before the attempt settles, so its result carries only the step-time evictions. The docs say so.report-v1.schema.jsongets both enums plusallOfrules (reason only with an outcome, no reason onwritten, each reason only with its own outcome). Both fixtures and a schema test pin those rules.docs/cache.mdx, plusdocs/reference/agent.mdxandskills/e2e/references/agent.md.evicts an entry that did not serve a pass with nothing to record in its place, recorded the replayed tap afterbegin(), so it was going down the repair branch instead. Its host now records the tap through the grammar, as the real dispatch does, and the test assertsnot-replaced.Verified
Ran it locally: yes
e2erun against the browser fixture app with a scripted two-tap executor andcache: 'read-write'. Run 1 records, run 2 replays, and run 3 tightens the assertion so it fails. This isstep.cachefrom each run'sreport.json:trace-cache-replayandagent-trace-cachenow assert the outcome end to end: kept/confirmedon whole replays,writtenon re-recordings, evicted/unconfirmedwhen a later assertion fails, and no outcome when the failure is a model that never answered or the cache is read-only in CI.e2epackage run has failures in 26 files on Windows (CRLF line endings,\paths, POSIX file modes, git/telemetry environment). All 26 fail the same way onmain, and none of them touch the cache or the report.pnpm docs:checkwas not run locally, because it needs the Mintlify docs workspace installed.Checklist
pnpm changeset) for any change to a published package, or the change is testbed, docs, or CI only.docs/**/*.mdxpage for the behavior, andskills/e2e/if the skill describes it..d.tsreviewed,tests/types/sdk-types.tsupdated; wire change edits the schema and both fixtures. (sdk-types.tshas noStepCacheInfoassertions, so nothing changed there.)e2e/engine): not touched.