Skip to content

fix(tier3): propagate Harbor dual-arm task suffixes and normalize canonical case ID resolution - #161

Open
kweinmeister wants to merge 7 commits into
NVIDIA:mainfrom
kweinmeister:fix/harbor-task-arm-naming
Open

kweinmeister wants to merge 7 commits into
NVIDIA:mainfrom
kweinmeister:fix/harbor-task-arm-naming

Conversation

@kweinmeister

Copy link
Copy Markdown
Contributor

Summary

When running Tier 3 Harbor dual-arm (with-skill / without-skill) evaluations, native staged task.toml files retained their base [task] name without the arm suffix, and result collection could fail to correlate paired trials when Harbor produced external namespace prefixes (repo/, org__), attempt suffixes (__attempt-N, -attempt-N), or arm suffixes (-with-skill, -without-skill) in varying orders.

This change:

  • Propagates arm suffixes to task.toml: Updates _rewrite_task_toml and copy_native_tasks_with_skill_mode in src/skillevaluator/tier3/harbor/adapter.py (wired from src/skillevaluator/tier3/harbor/runner.py) so [task] name in staged native task.toml files includes -with-skill / -without-skill alongside the staged task directory name.
  • Commutative canonical case ID resolution: Updates src/skillevaluator/tier3/harbor/collector.py (_strip_attempt_suffix, _strip_arm_suffix, _canonical_case_id) to strip external repository/namespace prefixes and commutatively strip attempt and arm suffixes in any order while preserving legitimate case IDs (such as those in expected_ids, retaining skillevaluator-, or ending in -with / -without).
  • Fail-fast validation & tests: Validates arm_suffix inputs across adapter APIs and adds parameterized unit and integration tests in tests/test_tier3_public_runtime.py.

Verification

  • I am familiar with the Contributing Guidelines
  • Added or updated focused tests
  • Updated documentation for user-visible changes
  • Ran make lint
  • Ran make test
  • Ran make build
  • Did not add credentials, private datasets, or proprietary benchmark content

Release Impact

  • No user-visible release note needed
  • Updated CHANGELOG.md

…ll standards

Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
…ution

- Propagate arm suffixes to `[task] name` in staged native `task.toml` files.
- Decouple arm-suffix and attempt-suffix stripping with a commutative pipeline.
- Strip external repository and namespace prefixes before canonicalizing IDs.
- Protect case IDs retaining `skillevaluator-` or ending in `-with`/`-without`.
- Enforce fail-fast runtime string validation for `arm_suffix` across adapter APIs.
- Parameterize test suite and expand test matrix to cover all suffix variants.
- Document dual-arm evaluation fixes in CHANGELOG.md.

Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
Signed-off-by: Karl Weinmeister <kweinmeister@google.com>

# Conflicts:
#	CHANGELOG.md

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kweinmeister Thanks for the contribution. The current normalization introduces cross-case identity collisions, and the native name rewrite can corrupt valid TOML. Please address the three inline findings with regressions before approval.

Local validation: 418 focused runtime, case-ID, metrics, adapter, collector, and security-attribution tests passed; Ruff passed. Separate reproductions exposed the reported gaps. All 17 reported CI checks pass.

There is also a merge conflict with main in CHANGELOG.md. Could you please resolve it so the updated PR can complete verification and move toward merge?

if candidate in expected_case_ids:
return candidate

return _strip_arm_and_attempt_suffixes(name_part.removeprefix("skillevaluator-"))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve authored IDs when attributing security findings

This fallback strips legitimate authored IDs even without an expected-ID set. _annotate_security_attribution() calls _entry_id() without that set, so distinct valid cases such as case-1 and case-1-with-skill (or skillevaluator-case-1) collapse together. An unsafe action unique to the latter case is then classified as likely_baseline_prompt_or_environment when the unrelated case-1 baseline has the same finding; the base correctly reports likely_skill_related. Preserve authoritative reward entry IDs and pass the original case mapping through attribution instead of unconditionally removing recognizable text. Add a cross-case attribution regression.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_entry_id now keeps authoritative reward entry_id values intact and only strips attempt suffixes, while _canonical_case_id only strips skillevaluator- prefixes or arm suffixes when expected_case_ids is provided. I also threaded expected_case_ids through _annotate_security_attribution and _save_trials, and added a cross-case attribution regression test.

Comment on lines +4245 to +4246
if old_name.endswith(arm_suffix):
return m.group(0)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Keep authored suffixes distinct from staging suffixes

These tasks have just been copied from their authored source, so an existing suffix does not mean staging already appended it. Valid native names nvidia/case-1 and nvidia/case-1-with-skill both become nvidia/case-1-with-skill in the with-skill arm. With custom-only Harbor rewards, collection then assigns both attempts to the latter expected case and leaves case-1 missing. Append a staging suffix consistently to every source name and retain an unambiguous original-ID mapping for collection; cover a dual-arm run containing both authored IDs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_append_native_task_name_suffix now always appends the arm suffix when staging, so nvidia/case-1 and nvidia/case-1-with-skill stay distinct in the with-skill arm, and writes [metadata].entry_id when not already set. On the collection side, _extract_rewards and _entry_id_from_harbor_result take the arm suffix and strip at most one staging suffix from task_name. Added a dual-arm custom-only regression test covering both case IDs.

return
content = task_toml.read_text(encoding="utf-8")

pattern = r'(?ms)(\[task\]\s*?\n(?:(?!\[)[^\n]*\n)*?\s*name\s*=\s*)(["\'])(.*?)\2'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Update the native task name structurally

The regex does not preserve valid TOML syntax or reliably target [task].name. For example, name = """nvidia/case-1""" is accepted by Harbor but becomes name = "-with-skill""nvidia/case-1""", which raises TOMLDecodeError. A valid [task] # task identity header also misses this pattern; if [metadata].name appears earlier, the fallback modifies that field and leaves the task name unchanged. Parse the document and update only task.name, preserving unrelated fields, and add cases for triple-quoted strings and commented section headers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced the regex substitution in _append_native_task_name_suffix with tomllib.loads parsing and a token-aware value-span scanner for [task].name. It handles single and triple-quoted strings, inline comments on [task] headers, multiline arrays, and preceding [metadata].name or inline tables, then validates the updated document with tomllib.loads before writing. Added parameterized tests for these TOML variants.

Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
@rng1995

rng1995 commented Sep 26, 2026

Copy link
Copy Markdown
Collaborator

Resolved the merge conflict with main and pushed 4ff18f5. Both the PR's release note and the new upstream entries are preserved. All non-changelog files were verified unchanged from their respective PR/main versions.

Local verification: 48 case-ID/report dependency tests passed; diff checks and Fern validation passed (authenticated redirects check skipped). The three code-review findings remain open, so this is not an approval.

Signed-off-by: Karl Weinmeister <kweinmeister@google.com>

# Conflicts:
#	CHANGELOG.md
…tructurally

Signed-off-by: Karl Weinmeister <kweinmeister@google.com>
@kweinmeister

Copy link
Copy Markdown
Contributor Author

Merged main and pushed updates for the review comments:

  • Authoritative reward entry_id values are preserved without stripping arm suffixes or skillevaluator- prefixes, and _canonical_case_id only strips generated wrappers when matching against expected_case_ids, which is now passed through _annotate_security_attribution and _save_trials.
  • Native staging always appends one arm suffix to [task].name and records [metadata].entry_id when absent, while reward extraction strips at most one known arm suffix from task_name.
  • [task].name is updated via a token-aware TOML scanner and validated with tomllib.loads so triple-quoted strings, commented [task] headers, and [metadata].name fields are handled cleanly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants