fix(ci): collect trigger types instead of trigger instances#73
Conversation
skills/v0.1.3 published with triggers=[] even though the worker exposes two trigger types (`skills::on-change`, `prompts::on-change`). collect_worker_interface was calling `engine::triggers::list`, which returns trigger INSTANCES (subscriptions registered by other workers), not the trigger TYPES the worker itself exposes for others to subscribe to. With only the target worker connected during publish, the instances list is empty by definition. Switch the live collection to `engine::trigger-types::list` (with `include_internal: false` so the engine::* types are filtered out by the engine itself) and map each TriggerTypeInfo entry to the registry trigger schema: - `id` → `name` (e.g., `skills::on-change`) - `description` → `description` - `trigger_request_format` → `invocation_schema` - `call_request_format` → `return_schema` Drop `_derive_trigger_name` / `_slug` / the instance-shape fields in `_normalize_registry_trigger`: the live path now produces registry-shape entries directly, and the `build_payload` interface.json passthrough only needs to enforce the registry shape.
📝 WalkthroughWalkthroughThe pull request refactors trigger collection and normalization in build scripts from processing trigger instances to processing trigger types. The worker-interface pipeline now calls ChangesTrigger Type-Based Processing Pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/scripts/build_publish_payload.py (1)
165-171: 💤 Low valueConsider filtering trigger types by worker namespace for consistency with functions.
The function processing (lines 151-163) explicitly filters functions by
worker_function_idsfrom the worker object. However, trigger types are collected without verifying they belong to the current worker—the code relies on the deployment-time assumption that only the target worker is connected.If multiple workers are ever connected during collection (e.g., in a different CI scenario), this could include trigger types from unrelated workers. Consider filtering by namespace prefix (e.g.,
{worker_namespace}::) similar to how functions are matched, or document the assumption explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/build_publish_payload.py around lines 165 - 171, The trigger collection currently appends every registry trigger type without ensuring it belongs to the current worker; update the loop that iterates _extract_array(trigger_types_json, "trigger_types") to only consider trigger types whose id (tt_id = trigger_type.get("id")) is a string and begins with the current worker namespace prefix (e.g., f"{worker_namespace}::"), skipping others (in the same way functions are filtered by worker_function_ids), then call _normalize_registry_trigger_type and append; reference the variables and functions triggers, trigger_types_json, _extract_array, tt_id, and _normalize_registry_trigger_type to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/scripts/build_publish_payload.py:
- Around line 165-171: The trigger collection currently appends every registry
trigger type without ensuring it belongs to the current worker; update the loop
that iterates _extract_array(trigger_types_json, "trigger_types") to only
consider trigger types whose id (tt_id = trigger_type.get("id")) is a string and
begins with the current worker namespace prefix (e.g., f"{worker_namespace}::"),
skipping others (in the same way functions are filtered by worker_function_ids),
then call _normalize_registry_trigger_type and append; reference the variables
and functions triggers, trigger_types_json, _extract_array, tt_id, and
_normalize_registry_trigger_type to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8cadd5b2-9a9b-454f-8eef-b3c3761299e4
📒 Files selected for processing (2)
.github/scripts/build_publish_payload.py.github/scripts/collect_worker_interface.py
Summary
skillsv0.1.3 published successfully (run) but withtriggers=[], even though the worker exposes two trigger types:collect_worker_interface.pywas callingengine::triggers::list, which returns trigger instances (subscriptions some other worker has registered), not the trigger types the worker itself defines. Inside the publish job only the target worker is connected, so the instances list is empty by construction — and trigger types we actually want to publish never show up.Fix
collect_worker_interface.py: queryengine::trigger-types::listwithinclude_internal: false(engine drops its ownengine::*types).build_publish_payload.py: mapTriggerTypeInfo(defined in iii-hq/iii engine_fn/mod.rs:121-128) to the registry trigger schema:id→name(e.g.skills::on-change)description→descriptiontrigger_request_format→invocation_schemacall_request_format→return_schema_derive_trigger_name/_slugand the instance-shape fields in_normalize_registry_trigger. The collect path emits registry-shape entries directly, sobuild_payload's passthrough now just enforces the schema.Test plan
create-tag.ymlforskillswithtag=next.Collect worker interfacestep printstriggerscontaining two entries with namesskills::on-changeandprompts::on-change.POST /publishreturns 200, registry shows both trigger types on the skills entry.triggersstays empty, no regression.Summary by CodeRabbit