Skip to content

refactor(sandbox)!: forward any attribution key as sandbox tags instead of a fixed trio - #49

Closed
polakamtejas wants to merge 1 commit into
tejas/remove-project-idfrom
tejas/generic-attribution-tags
Closed

polakamtejas wants to merge 1 commit into
tejas/remove-project-idfrom
tejas/generic-attribution-tags

Conversation

@polakamtejas

@polakamtejas polakamtejas commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #45; retarget to main once #45 merges.

This finishes the move to generic attribution: core no longer names any attribution dimension. The Modal and E2B providers kept only product, customer and team from a sandbox's attribution (_build_cost_attribution_tags), and every other key was silently dropped. That fixed set was kept on purpose when attribution became an open dict[str, str], so emitted billing labels stayed byte-identical for one deployment. In an open-source core it's just three names that nothing else defines.

Modal: App tags vs sandbox tags

  • App tags (_build_app_tags) are the deployment-wide config.toml [sandbox.attribution] defaults only, under any keys. Modal's billing report breaks cost down by App tags. Since refactor(attribution)!: remove project_id and the LiteLLM cost attribution built on it #45 one App serves every run, and its tags are set on first lookup, so a run's own values there were really the first run's.
  • Sandbox tags (_build_sandbox_tags) are the run's full attribution, with unset keys filled from [sandbox.attribution]. That includes pipeline_step and run_id from the deploy step; a task-authored value still wins, as before. Before, sandbox tags carried only pipeline_step and run_id.
  • Keys and values are made Modal-valid with the existing _modal_tag_value: [a-zA-Z0-9._-], at most 63 characters (Modal's check_tag_dict). An unset or empty value is left out, since Modal has no empty tag. An explicit empty value still isn't replaced by the default.
  • The same builders are used by modal_vm.

E2B

  • Sandbox metadata carries the same resolved attribution under any keys (_attribution_metadata). E2B no longer imports a Modal-private helper.
  • agent_env_exposed_ports stays agent-env's own: an attribution key of that name is never written, with or without exposed ports, so reconnect can't misread it.

Log line

  • _log_sandbox_started used to spread the sandbox tags into the log record's extra. With arbitrary keys, one named name or message would raise from LogRecord. The full set is now under modal_sandbox_tags; pipeline_step and run_id stay top-level, so the existing container-id join is unchanged.

Docs: one sentence in AGENTS.md (providers row), and a commented [sandbox.attribution] example in .agentenv/config.example.toml.

Breaking for downstream

  • Modal App tags no longer carry product / customer / team from a caller's attribution. They carry exactly what [sandbox.attribution] configures, so a deployment that billed on those keys sets them there.
  • Per-run attribution moves to Modal sandbox tags, which, per the existing note in modal_sandbox.py, don't appear in Modal's billing report.
  • Modal sandbox tags and E2B metadata now include every attribution key and configured default, not only the trio or the run/step keys.
  • The started-sandbox log line nests extra keys under modal_sandbox_tags.
  • check_plugin_api.py reports no plugin-surface break; the ! is for the emitted-tag change.

Testing

  • attribution_wire_test.py is rewritten for the generic wire, with neutral sample keys (group, owner, batch):
    • App tags = the configured defaults (env references included), empty without config, shared with modal_vm
    • any key reaches the sandbox tags
    • an explicit value beats the default
    • an empty value emits no tag
    • no attribution and no defaults → no tags
    • keys and values made Modal-valid
  • modal_sandbox_tags_test.py: sandbox tags carry every key. A provider-level test checks that create_container gives _get_app the configured defaults only, while the sandbox gets the run's attribution over them.
  • e2b_sandbox_provider_test.py: arbitrary keys pass through, configured defaults fill unset keys, and an attribution agent_env_exposed_ports never lands, with or without ports.
  • modal_sandbox_started_log_test.py: a tag named name logs without clashing. It fails against the old spreading code.
  • Other tests that used team only as a sample key now use group. test_the_quartet_is_no_longer_a_keyword_on_any_backend keeps team= on purpose: it checks the old keyword is refused.
  • pytest tst/unit packages/agentenv-protocol/tests -n auto: 5783 passed. The only failures are the 2 test_mcp_cli_builder tests, which fail the same way on main (the generated CLI's interpreter has no click).
  • git grep -nE '"(product|customer|team)"' -- src is empty.

🤖 Generated with Claude Code

RetriggerConfidence Score: 2/5

This PR is not ready to merge because Modal can lose attribution labels, reject an empty tag key, or keep old App billing labels.

Fix All in CursorFindings

  1. P1 Modal drops an attribution label ▶
  2. P1 Empty key stops a sandbox ▶
  3. P1 Old App tags remain ▶
Fix with agent prompt
### Issue 1
src/agent_env/providers/sandbox_providers/modal_sandbox.py:420
If a run has both `billing team` and `billing/team`, `_modal_tag_value` turns both keys into `billing-team`. The later value silently replaces the earlier one, so the run loses a label. Reject keys that collide or give them distinct tag names.

### Issue 2
src/agent_env/providers/sandbox_providers/modal_sandbox.py:420-422
If a step supplies `{"": "value"}`, `_modal_tags` keeps the empty key because it filters only empty values. The key then reaches Modal even though the tag format requires at least one character, so Modal can reject sandbox creation. Reject empty keys before sending tags.

### Issue 3
src/agent_env/providers/sandbox_providers/modal_sandbox.py:407
When `[sandbox.attribution]` has no defaults, `_build_app_tags` returns `{}`. Both Modal providers skip `set_tags` for an empty dictionary, so an existing App keeps tags from earlier runs or an earlier configuration. After an upgrade or removal of defaults, Modal can keep grouping billing under those old labels. Clear the App's tags when the configured set is empty.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

Modal and E2B now carry attribution under any key, filling values from [sandbox.attribution] when a run leaves them unset. Modal keeps deployment defaults on App tags and run values on sandbox tags; E2B writes them to sandbox metadata.

  • Modal nests the full tag set in started-sandbox logs while keeping pipeline_step and run_id at the top level.
  • E2B keeps its exposed-ports metadata separate from attribution.
  • The config example, provider docs, and test samples now describe or use generic attribution keys.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  C["[sandbox.attribution] defaults"] --> A["Modal App tags"]
  C --> M["Merge with run attribution"]
  R["Run attribution"] --> M
  M --> S["Modal sandbox tags"]
  M --> E["E2B metadata"]
Loading

Reviews (1) · Last reviewed commit: "refactor(sandbox)!: forward any attribut..."

…ad of a fixed trio

The Modal and E2B providers kept only product, customer and team from a
sandbox's attribution, so every other key was dropped. That fixed set was kept
when attribution became an open dict, so emitted billing labels stayed
byte-identical for one deployment; core no longer names any dimension.

Modal App tags are now the deployment-wide [sandbox.attribution] defaults
only: one App serves every run and its tags are set on first lookup, so a
run's values there were the first run's. A run's attribution, unset keys
filled from those defaults, goes on each sandbox's tags with keys and values
made Modal-valid; an empty value is left out, since Modal has no empty tag.
E2B sandbox metadata carries the same resolved attribution, and an
attribution key cannot stand in for agent-env's exposed-ports record. The
sandbox-started log line nests the full tag set under modal_sandbox_tags, so
an arbitrary key cannot clash with a LogRecord attribute; pipeline_step and
run_id stay top-level.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@polakamtejas
polakamtejas requested a review from a team as a code owner October 2, 2026 03:29
key: _modal_tag_value(attribution[key])
for key in (PIPELINE_STEP_KEY, RUN_ID_KEY)
if attribution.get(key)
_modal_tag_value(key): _modal_tag_value(str(value))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Modal drops an attribution label
If a run has both billing team and billing/team, _modal_tag_value turns both keys into billing-team. The later value silently replaces the earlier one, so the run loses a label. Reject keys that collide or give them distinct tag names.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agent_env/providers/sandbox_providers/modal_sandbox.py
Line: 420

Comment:
**Modal drops an attribution label**
If a run has both `billing team` and `billing/team`, `_modal_tag_value` turns both keys into `billing-team`. The later value silently replaces the earlier one, so the run loses a label. Reject keys that collide or give them distinct tag names.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Cursor Fix in Claude Code Fix in Codex

Comment on lines +420 to +422
_modal_tag_value(key): _modal_tag_value(str(value))
for key, value in attribution.items()
if value is not None and str(value) != ""

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Empty key stops a sandbox
If a step supplies {"": "value"}, _modal_tags keeps the empty key because it filters only empty values. The key then reaches Modal even though the tag format requires at least one character, so Modal can reject sandbox creation. Reject empty keys before sending tags.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agent_env/providers/sandbox_providers/modal_sandbox.py
Line: 420-422

Comment:
**Empty key stops a sandbox**
If a step supplies `{"": "value"}`, `_modal_tags` keeps the empty key because it filters only empty values. The key then reaches Modal even though the tag format requires at least one character, so Modal can reject sandbox creation. Reject empty keys before sending tags.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Cursor Fix in Claude Code Fix in Codex

}
Modal's billing report breaks cost down by App tags, and one App serves every run with its
tags set on first lookup, so per-run attribution goes on sandbox tags instead."""
return _modal_tags(apply_default_attribution({}))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Old App tags remain
When [sandbox.attribution] has no defaults, _build_app_tags returns {}. Both Modal providers skip set_tags for an empty dictionary, so an existing App keeps tags from earlier runs or an earlier configuration. After an upgrade or removal of defaults, Modal can keep grouping billing under those old labels. Clear the App's tags when the configured set is empty.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agent_env/providers/sandbox_providers/modal_sandbox.py
Line: 407

Comment:
**Old App tags remain**
When `[sandbox.attribution]` has no defaults, `_build_app_tags` returns `{}`. Both Modal providers skip `set_tags` for an empty dictionary, so an existing App keeps tags from earlier runs or an earlier configuration. After an upgrade or removal of defaults, Modal can keep grouping billing under those old labels. Clear the App's tags when the configured set is empty.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Cursor Fix in Claude Code Fix in Codex

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.

1 participant