Repository navigation
fix(tier3): exclude failed skill reads from execution credit - #201
Conversation
Signed-off-by: YMuskrat <Y.Laaouach@sussex.ac.uk>
Signed-off-by: YMuskrat <Y.Laaouach@sussex.ac.uk>
Signed-off-by: YMuskrat <Y.Laaouach@sussex.ac.uk>
Signed-off-by: YMuskrat <Y.Laaouach@sussex.ac.uk>
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
Automated review: 3 findings (0 P1, 1 P2, 2 P3); 3 posted inline.
Signed-off-by: YMuskrat <101849520+YMuskrat@users.noreply.github.com>
Signed-off-by: YMuskrat <101849520+YMuskrat@users.noreply.github.com>
Require matching tool result IDs before treating a read as failed. Respect shell -- delimiters and stdin operands when identifying isolated reads. Cover both host and verifier, including actual shell output and Claude log conversion. Validation: 571 focused tests passed; make lint and make build passed. Full-suite attempts stalled in this environment on both this branch and unchanged main; a full-suite pass is not claimed. Signed-off-by: YMuskrat <101849520+YMuskrat@users.noreply.github.com>
128e184 to
943c655
Compare
chrisknvidia
left a comment
chrisknvidia
left a comment
There was a problem hiding this comment.
Reviewed 943c655869b052ac2db6f0d6764cb7f93b837745: not ready to merge. The failed native Skill-call handling and OpenCode fallback/changelog comments are addressed. The compound-read comment is only partially addressed: a real brace-expanded multi-file read still loses successfully earned skill credit. I added the reproduced P2 case to the existing thread to avoid duplicating it: #201 (comment).
Verification: 2,715 focused tests passed with Harbor 0.24.0. The remaining regression was independently reproduced with real Bash stdout/stderr through both native Harbor and fallback Claude converters, then both host and verifier graders, and compared with main. No live provider/agent E2E or full-suite pass is claimed. Current-head CI has 15 successful checks; Python 3.12 and Python 3.13 test jobs remain in progress at this review.
A single shell operand such as `cat skills/{demo-skill,missing}/SKILL.md`
or `cat skills/*/SKILL.md` can expand into several files. The command
prints SKILL.md and then exits 1 for the other file, but it was counted
as one isolated read, so the failure removed activation, workflow and
routing credit.
A shell failure now cancels a read only when the operand has no brace,
glob, `$` or backtick, so it names exactly one file. Expanding operands
keep their previous credit. Tilde paths still count as one file, since
tilde expansion always yields one path. Host checks and the Harbor
verifier share the rule, and both are covered by a real brace-expansion
run.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Resolve the CHANGELOG.md conflict with NVIDIA#188 and NVIDIA#193 by keeping both sides' Fixed entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Resolve the CHANGELOG.md conflict with NVIDIA#199 by keeping both sides' Fixed entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Keep the structured-content helpers from main alongside the failed-read error helper. The merged code preserves the shell-expansion fix from 7481975. Validation: 500 focused tests passed; make lint and make build passed. The full test attempt timed out after 90 seconds, so no full-suite pass is claimed. Signed-off-by: YMuskrat <101849520+YMuskrat@users.noreply.github.com>
|
Thank you for the review, appreciated. I Merged the latest main and sorted out the changelog/import conflicts. Kept both changelog entries and #203's structured-answer helpers, with the shell-expansion fix from The combined version passed 500 focused tests, lint and the package build. The full suite hit the 90s time limit here, so I'm not claiming a full pass. Added the DCO sign-off to the cleanup commit and checked that all non-merge commits are signed off. |
Resolve the CHANGELOG.md conflict with NVIDIA#196 by keeping both sides' Fixed entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
No conflicts; brings the branch up to date with main after NVIDIA#182. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
Re-reviewed at 866abb0. All three review threads are addressed: failed isolated SKILL.md reads and failed Skill calls lose credit, compound or expanding shell reads keep their credit, and failed reads in rebuilt OpenCode trajectories are flagged, with host and verifier in parity. The merges with main kept #203's structured-content helpers. The full local suite and all 17 CI checks pass.
Brings the branch up to date with main after NVIDIA#190. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Brings in #182 (Kata Containers backend), #190 (criterion-derived accuracy) and #201 (failed skill-read credit). Conflict: src/skillevaluator/tier3/harbor/runner.py. Main imports Iterable for the new _staged_task_environment_error() check and rejects staged with-skill tasks that the selected environment cannot run. This branch imports Sequence and collects every staged task path so the runtime context can be checked against the publication target. Resolution: import Iterable, Iterator, Mapping and Sequence, and keep both steps for each agent's with-skill tasks. The staged paths are collected first, then main's environment check runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
3 participants
Summary
A
Readof the expected SKILL.md currently earns a 1.0 execution score even when Claude explicitly reports that the file does not exist. Preserveis_erroras Harbor-compatible result metadata, carry it through tool normalization with call ownership intact, and exclude explicitly failed document reads from activation, workflow and routing credit in the host and standalone verifier.Successful reads and retries retain their credit, including instruction text that discusses errors. Only results matched to the tool call can mark it as failed. Compound reads and extra file operands (including dash-prefixed files after
--and stdin-) keep their previous credit.Closes #200
Latest verification: 571 focused tests passed. This includes host/verifier regressions for unrelated error results and real shell reads that print SKILL.md before another file fails.
Verification
make lint(passed)make test(attempted; stalled and interrupted, not a full pass)make build(wheel and source distribution passed)Verified with the locked dependencies on Python 3.12.
make lintandmake buildpassed. Full-suite attempts had failures and then stalled in this environment on both this branch and unchanged main (098f96a); both were interrupted. The latest full suite is not claimed green, and its failures have not been fully compared.Release Impact