[AAASM-4301] 🐛 (assembly): Validate agent_id at init_assembly entry#223
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Chisanan232
left a comment
Chisanan232
left a comment
There was a problem hiding this comment.
Claude Code Review — AAASM-4301
CI: all green (SonarCloud, codecov/patch, unit + integration on 3.13, LangChain contract, pip-audit, CodeQL — all pass).
Scope coverage: matches the ticket's Fix section exactly.
- Adds
_AGENT_ID_RE = re.compile(r"^[A-Za-z0-9_.-]{1,128}$")at module scope. - Adds
_validate_agent_id(agent_id: str) -> strhelper that raisesValueErroron non-match. - Wraps
agent_id or _DEFAULT_AGENT_IDin_validate_agent_id(...)at the single call site ininit_assembly(). - No touches to
runtime_interceptor.py, no new deps, no test additions, no drive-by refactors. Aligns with the surgical scope declared in the PR body.
Side effects — verified:
_DEFAULT_AGENT_ID = "agent-assembly-default"(22 chars,[a-z-]only) matches the regex — default path continues to work.- Grepped
test/for everyinit_assembly(agent_id=...)call and every explicitagent_id="..."literal that flows toinit_assembly: all values ("test-agent-001","agent-x","agent-a","cp-agent","single-host-agent","env-gw-agent","child-agent","unicode-child",f"agent-{mode}"formode ∈ {auto,ebpf,proxy,sdk-only},"bench-agent","ignored-at-this-layer", etc.) are alphanumeric +-, well under 128 chars — all pass the new validator. - The
test_init_assembly_forwards_ambient_spawn_parent_on_registerunicode test passes unicode viateamandparent_agent_idonly;agent_id="unicode-child"itself is ASCII. - Public behavior change is exactly what the ticket describes: malformed
agent_idnow fails fast with a clearValueErrorinstead of a downstreamFileNotFoundError. Documented as intentional in the PR body's Breaking Changes section.
Commit hygiene:
✨ (assembly): Add _AGENT_ID_RE regex for agent_id validation
✨ (assembly): Add _validate_agent_id helper
🐛 (assembly): Enforce agent_id validation in init_assembly (AAASM-4301)
Three commits, all <emoji> (<scope>): <summary>, well-granulated per the global commit policy (one regex constant, one helper function, one wiring change). No Co-Authored-By / Generated-with-Claude trailers.
Verdict: APPROVE
|
Chisanan232
left a comment
Chisanan232
left a comment
There was a problem hiding this comment.
Claude Code Review — AAASM-4301 (follow-up: regression tests added)
Per Bryant's global rule "All bug fixes require a regression test" — added 2 test commits to this branch to cover the new _validate_agent_id behavior.
New commits
| SHA | Subject | What it covers |
|---|---|---|
ad30d22 |
✅ (assembly): Add parametrized tests for _validate_agent_id (AAASM-4301) | Two parametrized tests directly against the validator: 9 accept cases (including the module default, single-char boundaries, max-length 128) and 8 reject cases (empty, over-length, path-traversal, whitespace, path separator, shell metachar, non-ASCII, newline). |
1a573cf |
✅ (assembly): Add init_assembly regression tests for agent_id validation (AAASM-4301) | Two integration tests through init_assembly: (1) malformed agent_id="../attack" raises ValueError with the expected message; (2) the default path (no agent_id arg) resolves to _DEFAULT_AGENT_ID and passes the validator — guards against a future change that alters the default to a value the regex rejects. |
Both use the existing monkeypatch fixture pattern from the surrounding tests in test/unit/test_assembly.py to stub out _register_adapters and _start_network_layer, so no real runtime connection is attempted.
Commit granularity
Per Bryant's "One test is one git commit" rule I split by concern: the pure validator tests are one commit (they're on the same helper, exercised via parametrize), and the init_assembly integration tests are the next commit (they're on a different unit of behavior). Both subjects follow <GitEmoji> (<scope>): <summary>.
CI after test push
All 19 hard checks green (unit + integration on 3.13/ubuntu, LangChain contract test, pip-audit, CodeQL, SonarCloud, codecov/patch, Analyze (python), docs build). 7 checks skipped (deploy/release-gated jobs). Zero failures.
The 4 new tests all pass on the CI runners (confirmed via the pytest report in the unit-tests (3.13, ubuntu-latest) job).
Verdict (updated): still APPROVE — the fix now ships with the regression coverage it warranted.
1 participant
Description
Add input validation for the
agent_idparameter ininit_assembly()so malformed values are rejected with a clearValueErrorbefore they can flow into socket-path interpolation and produce cryptic downstreamFileNotFoundErrors.Introduces
_AGENT_ID_RE = re.compile(r"^[A-Za-z0-9_.-]{1,128}$")and a_validate_agent_id()helper.init_assembly()now wrapsagent_id or _DEFAULT_AGENT_IDin the validator so both explicit and default values are enforced._DEFAULT_AGENT_ID("agent-assembly-default") matches the regex.Scope is surgical: no changes to
runtime_interceptor.py, no new dependencies, no test additions, no refactor of surrounding code.Type of Change
Breaking Changes
Callers already passing well-formed IDs (alphanumeric,
_,.,-, <=128 chars) are unaffected. Callers passing malformed IDs will now see a fastValueErrorat init instead of a downstream failure — this is the intended behavior change.Related Issues
Testing
Per repo sweep conventions the CI in this repo exercises existing tests for
init_assembly(); new coverage for this validator will be added in a follow-up if the sweep triage requests it. The change is a narrow input-validation guard on a single call site.Checklist