Repository navigation
Conversation
Signed-off-by: Jessica Buhl <jbuhl@nvidia.com>
Signed-off-by: Jessica Buhl <jbuhl@nvidia.com>
|
Review of commit This PR adds a choice about saving detailed evaluation records, including the agent's inputs, responses, and tool activity. Later audits use those records to understand what happened. The workflow remembers the choice when work resumes or evaluations run again. flowchart TD
A[Approve evaluation suite] --> B{Can run records be saved?}
B -->|Optional| C[Ask whether and where to save]
B -->|Already automatic| D[Use existing recording]
B -->|Unsupported| E[Explain limits on later audits]
C --> F[Remember the choice]
D --> G[Confirm settings and permission to run]
E --> G
F --> G
G --> H[Run evaluations]
H --> I[Review results and available records]
I --> J[Offer an audit with separate permission]
[P2] Unsupported recording can block required setup. At This can strand a baseline evaluation that should run without saved records. Let the handoff accept resolved, unsupported recording as disabled and carry its limitation forward. Overall assessment: The change makes saving permission explicit and preserves it across resumed work. I'd request changes because this setup blocker affects an otherwise supported workflow. The review covered the pinned comparison across all 13 files, totaling 991 additions and 61 deletions. Tests were inspected but weren't executed. |
Keep saving disabled and carry capture limitations into the adapter handoff. Signed-off-by: Rob Rhyne <rrhyne@nvidia.com>
aleckhoury
left a comment
There was a problem hiding this comment.
Review of 0d3fb03.
Request changes. The core design is sound. The saving check-in is a separate stage, the state helper enforces an answer → settings-display ordering across turns, the choice never authorizes execution, changing the answer invalidates authorization without forcing a redisplay, and a new suite resets the choice. Two issues contradict the PR's stated guarantees:
- Saving capture settings on a completed suite (legacy, or after changing
choice/directory) flips it back to incomplete and orphans prior runs (inline onfirst_eval_state.py:633). - The integration handoff is now deferred until after suite approval, removing parallel engineering work during authoring (inline on
first_eval.py:119-120).
The prior P2 from @rrhyne (unsupported capture blocking the handoff) is resolved in 0d3fb03; capture_ready now uses trajectory_capture_resolved(), covered by test_unsupported_capture_allows_adapter_handoff_with_saving_disabled.
Verification
tests/test_first_eval_rendering.py,tests/test_first_eval_state.py,tests/test_skill_contract.pyat0d3fb03: 808 passed, 4 skipped. Full suite not run.git merge-treeagainst currentmain(54674ec): clean.- New doc anchors,
skills/gym-to-atif/SKILL.md, and the HarborTrajectory.model_validate_jsonreference (audit_spec/measure.py) all resolve. - Throwaway probes:
- Legacy suite completed, then
mode: automaticsaved →completeTrue→False,next_stageresults→execution_settings. - Suite completed with
choice: true, thenchoice: false→completeTrue→False,baselineno longer reviewable. - Suite completed, then only
directorychanged →completeTrue→False. - Rendered
trajectory_settingsfor automatic/unsupported andexecution_settingsfor declined; copy issues noted inline.
- Legacy suite completed, then
The docs are consistent across first-eval.md, the provider references, and suite-readme.md, and the Gym converter is correctly gated to enabled capture. The renderer copy needs a pass (inline P3s).
Keep capture preferences out of retained run identities and bind them to execution authorization and settings display instead. Restore the adapter handoff before suite approval with saving disabled while its choice is pending. Clarify capture disclosures and summaries, accept literal tildes in paths, and cover completed suites, new launch permissions, and early handoffs. Merge current main and retain its shared first-eval task tables. Signed-off-by: Rob Rhyne <rrhyne@nvidia.com>
Eval bootstrapping now asks whether to save local trajectories in a separate check-in after suite approval, before other execution settings. The prompt explains that Eval Author needs these records to measure coverage against
ETHOS.md. Scores alone are insufficient.Validation:
test_discover_exits_early_without_harbor_or_gymalso fails with the same phase mismatch on untouched PR commit0d3fb03.Fixes ASE-1265.