ci(harness-bundle): single-dispatch fan-out release for harness + deps - #110
Merged
Conversation
….toml and iii.worker.yaml
Companion to 49b5726, which committed the tracked-file portion of the iii-native restructure but missed these new untracked files. Also picks up shell/iii.worker.yaml's runtime/scripts metadata to match the other deps.
Adds release-harness-bundle.yml (fires on harness/v* tags, computes the publish set from harness/iii.worker.yaml deps that have a local version greater than what is currently registered, fans out builds and publishes through the existing _rust-binary.yml + _publish-registry.yml reusables) and create-tag-harness-bundle.yml (one workflow_dispatch that bumps harness, pre-bumps deps with source changes since their last tag, and pushes the umbrella harness/v<X.Y.Z> annotated tag). release.yml drops harness/v* from its tag patterns to avoid double-publishing — the bundle workflow owns it now. The original release.yml still serves single-worker releases for everything else. Pre-bumps approval-gate, provider-anthropic, provider-openai to 0.2.0 since each has substantive src changes since v0.1.0. ci.yml widens the metadata_globs allowlist with Cargo.toml so description-only Cargo.toml edits no longer flip pr-checks into strict mode (still strict on src/, build.rs, tests/, etc.). registry/index.json drops harness.default_config.workers — the harness binary's WorkerConfig only deserializes engine_url, so the workers array was dead config that "iii worker add harness" would persist and the harness would silently ignore. .gitignore adds harness/data/ for runtime sled state.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds harness bundle/release workflows, a harness-types crate with Serde data contracts, refactors approval-gate to use YAML-backed WorkerConfig and a clap/tracing CLI, standardizes worker runtimes/scripts across many workers with III_URL CLI env wiring, updates registry entries and CI release triggers, and removes the subagent worker and its documentation/tests. ChangesHarness bundle, approval-gate, and standardization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
harness/ARCHITECTURE.md (1)
101-109:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language to the fenced code block.
The block should declare a language (likely
bash) to satisfy markdown lint and improve rendering.🤖 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 `@harness/ARCHITECTURE.md` around lines 101 - 109, The fenced code block listing demo.sh commands needs a language specifier for markdown linting; update the opening fence from ``` to ```bash so the block reads as a bash snippet (affecting the block containing the demo.sh build/engine/start/verify/web/stop/all lines), ensuring syntax highlighting and markdown lint compliance..github/workflows/ci.yml (1)
74-81:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
Cargo.tomlis now over-classified as metadata-only.This downgrades strict PR checks for all manifest-only changes, including dependency/feature/runtime changes that should still require strict version-bump enforcement. Consider key-level handling (e.g., only
description,readme, authors/license metadata as soft) instead of file-level classification.🤖 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/workflows/ci.yml around lines 74 - 81, The current metadata_globs list treats "Cargo.toml" as a metadata-only change which is too coarse; remove "Cargo.toml" from metadata_globs and instead add a targeted check in the PR classification logic that parses Cargo.toml and treats only safe metadata keys (e.g., description, readme, authors, license) as metadata-only while marking dependency, version, features, or build/runtime key changes as non-metadata; update the code that references metadata_globs and the PR gating step to call this new Cargo.toml key-level validator so manifest edits that affect dependencies/features still trigger strict checks.
🧹 Nitpick comments (4)
policy-denylist/iii.worker.yaml (1)
9-14: 💤 Low valueLGTM: Standard Rust runtime configuration added.
The runtime and scripts configuration follows a consistent pattern across workers. The
cargo buildandcargo runcommands are appropriate for local development.If these scripts will be used for production builds, consider using
cargo build --releasefor optimized binaries.🤖 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 `@policy-denylist/iii.worker.yaml` around lines 9 - 14, The runtime config uses development commands; update the scripts entries 'install' and 'start' to use release builds for production by replacing 'cargo build' with 'cargo build --release' and 'cargo run' with 'cargo run --release' (or keep dev commands for local only), ensuring the 'runtime.kind' remains 'rust' and the YAML keys 'scripts.install' and 'scripts.start' are the ones you modify.provider-anthropic/crates/auth-credentials/src/lib.rs (1)
18-21: ⚡ Quick win
provider_extraalways serializes asnullwhen unset.Unlike the sibling
Optionfields,provider_extra: serde_json::Valuehas noskip_serializing_if, so a serialized OAuth credential always carries"provider_extra": null. Trivial to make consistent.♻️ Proposed change
- #[serde(default)] - provider_extra: serde_json::Value, + #[serde(default, skip_serializing_if = "serde_json::Value::is_null")] + provider_extra: serde_json::Value,The same change applies to
provider-openai/crates/auth-credentials/src/lib.rs.🤖 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 `@provider-anthropic/crates/auth-credentials/src/lib.rs` around lines 18 - 21, The provider_extra field currently serializes as null because it is typed as serde_json::Value with #[serde(default)]; change the field declaration for provider_extra to Option<serde_json::Value> and add #[serde(default, skip_serializing_if = "Option::is_none")] so it is omitted when unset; update the struct where provider_extra is defined (the provider_extra field) in both auth-credentials modules (provider_extra) to this Option type and serde attributes to make serialization consistent with the other optional fields.approval-gate/src/manifest.rs (1)
18-19: 💤 Low valueGood refactor to use WorkerConfig::default() serialization.
This approach is cleaner than hardcoding JSON and ensures the manifest stays in sync with the actual config struct.
Consider logging serialization failures instead of silently falling back to
{}, as this could hide configuration issues during development:📊 Optional improvement for debugging
- default_config: serde_json::to_value(crate::config::WorkerConfig::default()) - .unwrap_or_else(|_| serde_json::json!({})), + default_config: serde_json::to_value(crate::config::WorkerConfig::default()) + .unwrap_or_else(|e| { + eprintln!("WARN: WorkerConfig serialization failed: {e}"); + serde_json::json!({}) + }),🤖 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 `@approval-gate/src/manifest.rs` around lines 18 - 19, The current use of serde_json::to_value(crate::config::WorkerConfig::default()).unwrap_or_else(|_| serde_json::json!({})) silently swallows serialization errors; change the unwrap_or_else closure to capture the error and log it (e.g., using log::error or tracing::error) including the error details and context about serializing WorkerConfig::default() before returning the empty object fallback so failures are visible during development; keep the fallback serde_json::json!({}) to preserve behavior if desired.approval-gate/tests/manifest.rs (1)
5-11: ⚖️ Poor tradeoffConsider a more robust fallback path.
The hardcoded
target/debugpath may fail for release builds or custom target directories. Consider usingCARGO_TARGET_DIRor workspace metadata:🔧 Suggested improvement
fn binary_path() -> String { if let Some(path) = option_env!("CARGO_BIN_EXE_iii_approval_gate") { return path.to_string(); } - let manifest_dir = env!("CARGO_MANIFEST_DIR"); - format!("{manifest_dir}/target/debug/iii-approval-gate") + // Try CARGO_TARGET_DIR, fall back to workspace-relative target/ + let target_dir = std::env::var("CARGO_TARGET_DIR") + .or_else(|_| std::env::var("OUT_DIR").map(|p| { + std::path::PathBuf::from(p) + .ancestors() + .nth(3) + .unwrap() + .join("debug") + .display() + .to_string() + })) + .unwrap_or_else(|_| format!("{}/target/debug", env!("CARGO_MANIFEST_DIR"))); + format!("{target_dir}/iii-approval-gate") }Alternatively, document that the test requires
cargo testto be run from the workspace root, and the fallback is a known limitation.🤖 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 `@approval-gate/tests/manifest.rs` around lines 5 - 11, The fallback in binary_path() is brittle because it assumes "target/debug"; update binary_path to first try the existing option_env!("CARGO_BIN_EXE_iii_approval_gate"), then derive the target directory and profile instead of hardcoding "target/debug": read option_env!("CARGO_TARGET_DIR") (fallback to "target"), read env!("PROFILE") or option_env!("PROFILE") for "debug"/"release", and format the path as "{target_dir}/{profile}/iii-approval-gate"; keep the original CARGO_BIN_EXE check and, if you prefer, add a short comment documenting the remaining limitation or alternative to require running tests from workspace root.
🤖 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.
Inline comments:
In @.github/workflows/create-tag-harness-bundle.yml:
- Around line 213-245: The workflow currently commits and pushes the bump in the
"Commit bundle bumps" step before verifying tag availability in the "Tag and
push" step; change the order so the tag existence check (using the same
TAG="harness/v${HARNESS_VERSION}" and git rev-parse logic) runs before creating
or pushing the commit, and if the tag already exists exit with an error without
making the commit/push; update/remove the duplicate tag check in "Tag and push"
or keep it as a safety net but ensure the initial pre-commit check prevents
creating the orphaned bump commit when TAG (harness/v${HARNESS_VERSION}) already
exists.
In @.github/workflows/create-tag.yml:
- Around line 12-15: The generic tag workflow currently includes the list item
'harness' which must be removed so harness tags are handled only by
release-harness-bundle.yml; open the step that lists items including
'approval-gate', 'auth-credentials', 'harness', 'hook-fanout' and delete the
'harness' entry so create-tag-harness-bundle.yml and release-harness-bundle.yml
can enforce the dependency pre-bump logic correctly.
In @.github/workflows/release-harness-bundle.yml:
- Around line 44-55: The checkout step is not pinned to the intended tag,
causing the workspace to potentially be on a different commit than the tag
parsed later; update the initial actions/checkout@v4 step to include a ref that
uses the same tag expression as RAW_TAG (e.g. ref: ${{ inputs.tag ||
github.ref_name }}) so the checked-out commit matches the tag, keep fetch-depth:
0 and fetch-tags: true, and remove or keep the subsequent "Refetch annotated
tag" step as optional; ensure the RAW_TAG expression used in the git fetch
matches the ref used for actions/checkout to avoid metadata/code mismatches.
In `@approval-gate/README.md`:
- Line 88: Update the approval::resolve documentation row to use the consistent
identifier name: replace the tuple description "(session_id, tool_call)" with
"(session_id, function_call_id)" (keeping a note that tool_call_id is a legacy
alias only if needed); ensure any adjacent mentions in the same table row or
nearby text reference session_id and function_call_id so the README uses
function_call_id everywhere consistent with the rest of the docs.
In `@harness/ARCHITECTURE.md`:
- Around line 78-80: The document header "The 14 expected workers" and any
sample payload counts referencing 14 must be reconciled with the intro that says
13; update the heading and all sample payload counts to match the actual
EXPECTED_WORKERS value generated from iii.worker.yaml (and included into lib.rs
via build.rs) so the document consistently reflects the real worker count (13) —
search for "EXPECTED_WORKERS", the heading text, and any sample payload blocks
and change "14" to the correct number.
In `@harness/build.rs`:
- Around line 31-37: The worker extraction is too permissive (current chain
using skip_while/take_while/filter/filter_map/map) and treats nested YAML keys
like " version:" as worker names; tighten the filter that currently checks
starts_with(" ") && contains(':') to require an exact top-level dependency key
pattern — either replace that closure with a regex match like r"^
[A-Za-z0-9_-]+:" or explicitly check that the line starts with exactly two
spaces followed immediately by a valid key (alphanumeric, dash or underscore)
and a colon (e.g., ensure line.len() > 2, &line[0..2] == " ", then validate the
key portion before the colon), so only true top-level dependency entries under
"dependencies:" are captured (adjust the .filter and/or .filter_map around the
current filter_map(|l| l.split(':').next()) accordingly).
In `@harness/crates/harness-types/src/stream_event.rs`:
- Around line 123-131: The test named done_is_terminal is misnamed and only
asserts that AssistantMessageEvent::Stop { stop_reason: StopReason::End, ... }
returns false from is_terminal(), leaving the true path untested; update the
tests to (1) rename the existing test to reflect that Stop is non-terminal
(e.g., stop_is_non_terminal) and (2) add a new test that constructs a terminal
event (for example using AssistantMessageEvent::Done or an
AssistantMessage::default() converted into the terminal variant, or constructing
the variant that represents Done/Error) and asserts is_terminal() returns true;
reference AssistantMessageEvent::Stop, StopReason::End, is_terminal and the
terminal variant (Done/Error/AssistantMessage::default) when making the changes.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 74-81: The current metadata_globs list treats "Cargo.toml" as a
metadata-only change which is too coarse; remove "Cargo.toml" from
metadata_globs and instead add a targeted check in the PR classification logic
that parses Cargo.toml and treats only safe metadata keys (e.g., description,
readme, authors, license) as metadata-only while marking dependency, version,
features, or build/runtime key changes as non-metadata; update the code that
references metadata_globs and the PR gating step to call this new Cargo.toml
key-level validator so manifest edits that affect dependencies/features still
trigger strict checks.
In `@harness/ARCHITECTURE.md`:
- Around line 101-109: The fenced code block listing demo.sh commands needs a
language specifier for markdown linting; update the opening fence from ``` to
```bash so the block reads as a bash snippet (affecting the block containing the
demo.sh build/engine/start/verify/web/stop/all lines), ensuring syntax
highlighting and markdown lint compliance.
---
Nitpick comments:
In `@approval-gate/src/manifest.rs`:
- Around line 18-19: The current use of
serde_json::to_value(crate::config::WorkerConfig::default()).unwrap_or_else(|_|
serde_json::json!({})) silently swallows serialization errors; change the
unwrap_or_else closure to capture the error and log it (e.g., using log::error
or tracing::error) including the error details and context about serializing
WorkerConfig::default() before returning the empty object fallback so failures
are visible during development; keep the fallback serde_json::json!({}) to
preserve behavior if desired.
In `@approval-gate/tests/manifest.rs`:
- Around line 5-11: The fallback in binary_path() is brittle because it assumes
"target/debug"; update binary_path to first try the existing
option_env!("CARGO_BIN_EXE_iii_approval_gate"), then derive the target directory
and profile instead of hardcoding "target/debug": read
option_env!("CARGO_TARGET_DIR") (fallback to "target"), read env!("PROFILE") or
option_env!("PROFILE") for "debug"/"release", and format the path as
"{target_dir}/{profile}/iii-approval-gate"; keep the original CARGO_BIN_EXE
check and, if you prefer, add a short comment documenting the remaining
limitation or alternative to require running tests from workspace root.
In `@policy-denylist/iii.worker.yaml`:
- Around line 9-14: The runtime config uses development commands; update the
scripts entries 'install' and 'start' to use release builds for production by
replacing 'cargo build' with 'cargo build --release' and 'cargo run' with 'cargo
run --release' (or keep dev commands for local only), ensuring the
'runtime.kind' remains 'rust' and the YAML keys 'scripts.install' and
'scripts.start' are the ones you modify.
In `@provider-anthropic/crates/auth-credentials/src/lib.rs`:
- Around line 18-21: The provider_extra field currently serializes as null
because it is typed as serde_json::Value with #[serde(default)]; change the
field declaration for provider_extra to Option<serde_json::Value> and add
#[serde(default, skip_serializing_if = "Option::is_none")] so it is omitted when
unset; update the struct where provider_extra is defined (the provider_extra
field) in both auth-credentials modules (provider_extra) to this Option type and
serde attributes to make serialization consistent with the other optional
fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58b290da-8713-4797-aa96-11172c044654
⛔ Files ignored due to path filters (5)
approval-gate/Cargo.lockis excluded by!**/*.lockharness/Cargo.lockis excluded by!**/*.lockprovider-anthropic/Cargo.lockis excluded by!**/*.lockprovider-openai/Cargo.lockis excluded by!**/*.locksubagent/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (94)
.github/workflows/ci.yml.github/workflows/create-tag-harness-bundle.yml.github/workflows/create-tag.yml.github/workflows/release-harness-bundle.yml.github/workflows/release.yml.gitignoreREADME.mdapproval-gate/Cargo.tomlapproval-gate/README.mdapproval-gate/build.rsapproval-gate/iii.worker.yamlapproval-gate/skill.mdapproval-gate/skills/list_pending.mdapproval-gate/skills/policy_approval_gate.mdapproval-gate/skills/resolve.mdapproval-gate/src/config.rsapproval-gate/src/lib.rsapproval-gate/src/main.rsapproval-gate/src/manifest.rsapproval-gate/tests/integration.rsapproval-gate/tests/manifest.rsapproval-gate/tests/skill.rsauth-credentials/Cargo.tomlauth-credentials/iii.worker.yamlharness/ARCHITECTURE.mdharness/Cargo.tomlharness/Makefileharness/README.mdharness/build.rsharness/crates/harness-types/Cargo.tomlharness/crates/harness-types/src/agent_event.rsharness/crates/harness-types/src/agent_message.rsharness/crates/harness-types/src/content.rsharness/crates/harness-types/src/function.rsharness/crates/harness-types/src/lib.rsharness/crates/harness-types/src/stream_event.rsharness/crates/harness-types/src/thinking.rsharness/iii.worker.yamlharness/scripts/demo.shharness/src/lib.rsharness/tests/integration.rshook-fanout/Cargo.tomlhook-fanout/iii.worker.yamlhook-fanout/src/main.rsllm-budget/Cargo.tomlllm-budget/iii.worker.yamlllm-budget/src/main.rsmodels-catalog/Cargo.tomlmodels-catalog/iii.worker.yamloauth-anthropic/Cargo.tomloauth-anthropic/iii.worker.yamloauth-openai-codex/Cargo.tomloauth-openai-codex/iii.worker.yamlpolicy-denylist/Cargo.tomlpolicy-denylist/iii.worker.yamlprovider-anthropic/Cargo.tomlprovider-anthropic/crates/auth-credentials/Cargo.tomlprovider-anthropic/crates/auth-credentials/src/lib.rsprovider-anthropic/crates/overflow-classify/src/lib.rsprovider-anthropic/iii.worker.yamlprovider-anthropic/src/main.rsprovider-openai/Cargo.tomlprovider-openai/crates/auth-credentials/Cargo.tomlprovider-openai/crates/auth-credentials/src/lib.rsprovider-openai/crates/overflow-classify/src/lib.rsprovider-openai/iii.worker.yamlprovider-openai/src/main.rsprovider-router/Cargo.tomlprovider-router/iii.worker.yamlprovider-router/src/register.rsregistry/index.jsonsession-inbox/Cargo.tomlsession-inbox/iii.worker.yamlsession-tree/Cargo.tomlsession-tree/iii.worker.yamlsession-tree/src/main.rsshell/Cargo.tomlshell/iii.worker.yamlshell/src/main.rssubagent/Cargo.tomlsubagent/README.mdsubagent/build.rssubagent/iii.worker.yamlsubagent/skill.mdsubagent/skills/start.mdsubagent/src/config.rssubagent/src/lib.rssubagent/src/main.rssubagent/src/register.rssubagent/src/start.rssubagent/tests/integration.rssubagent/tests/manifest.rsturn-orchestrator/Cargo.tomlturn-orchestrator/iii.worker.yaml
💤 Files with no reviewable changes (14)
- subagent/skill.md
- subagent/build.rs
- subagent/iii.worker.yaml
- subagent/src/lib.rs
- subagent/src/register.rs
- subagent/src/start.rs
- subagent/skills/start.md
- subagent/tests/integration.rs
- subagent/Cargo.toml
- subagent/src/main.rs
- subagent/src/config.rs
- harness/tests/integration.rs
- subagent/README.md
- subagent/tests/manifest.rs
Comment on lines
+213
to
+245
| - name: Commit bundle bumps | ||
| env: | ||
| HARNESS_VERSION: ${{ steps.compute.outputs.harness_version }} | ||
| BUMPED_COUNT: ${{ steps.compute.outputs.bumped_count }} | ||
| run: | | ||
| git add -A | ||
| if git diff --cached --quiet; then | ||
| echo "::error::no changes to commit" | ||
| exit 1 | ||
| fi | ||
| git commit -m "chore(harness-bundle): release harness v${HARNESS_VERSION} (+${BUMPED_COUNT} deps)" | ||
| git push origin main | ||
|
|
||
| - name: Tag and push | ||
| env: | ||
| HARNESS_VERSION: ${{ steps.compute.outputs.harness_version }} | ||
| REGISTRY_TAG: ${{ inputs.registry_tag }} | ||
| BUMPED: ${{ steps.compute.outputs.bumped }} | ||
| run: | | ||
| TAG="harness/v${HARNESS_VERSION}" | ||
| if git rev-parse "$TAG" >/dev/null 2>&1; then | ||
| echo "::error::tag $TAG already exists" | ||
| exit 1 | ||
| fi | ||
| git tag -a "$TAG" -m "Release harness bundle ${TAG} | ||
|
|
||
| worker: harness | ||
| version: ${HARNESS_VERSION} | ||
| registry-tag: ${REGISTRY_TAG} | ||
| bumped-deps: ${BUMPED} | ||
| " | ||
| git push origin "$TAG" | ||
| echo "::notice::pushed $TAG (registry-tag=$REGISTRY_TAG)" |
There was a problem hiding this comment.
Check tag availability before pushing the bump commit.
Right now the workflow writes to main first and only then discovers that harness/v${HARNESS_VERSION} already exists. That leaves a partial release commit behind with no matching umbrella tag.
Suggested ordering change
+ - name: Check tag does not exist
+ env:
+ HARNESS_VERSION: ${{ steps.compute.outputs.harness_version }}
+ run: |
+ TAG="harness/v${HARNESS_VERSION}"
+ if git rev-parse "$TAG" >/dev/null 2>&1; then
+ echo "::error::tag $TAG already exists"
+ exit 1
+ fi
+
- name: Commit bundle bumps
env:
HARNESS_VERSION: ${{ steps.compute.outputs.harness_version }}
BUMPED_COUNT: ${{ steps.compute.outputs.bumped_count }}
run: |
@@
- - name: Tag and push
+ - name: Tag and push
env:
HARNESS_VERSION: ${{ steps.compute.outputs.harness_version }}
REGISTRY_TAG: ${{ inputs.registry_tag }}
BUMPED: ${{ steps.compute.outputs.bumped }}
run: |
TAG="harness/v${HARNESS_VERSION}"
- if git rev-parse "$TAG" >/dev/null 2>&1; then
- echo "::error::tag $TAG already exists"
- exit 1
- fi
git tag -a "$TAG" -m "Release harness bundle ${TAG}🤖 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/workflows/create-tag-harness-bundle.yml around lines 213 - 245, The
workflow currently commits and pushes the bump in the "Commit bundle bumps" step
before verifying tag availability in the "Tag and push" step; change the order
so the tag existence check (using the same TAG="harness/v${HARNESS_VERSION}" and
git rev-parse logic) runs before creating or pushing the commit, and if the tag
already exists exit with an error without making the commit/push; update/remove
the duplicate tag check in "Tag and push" or keep it as a safety net but ensure
the initial pre-commit check prevents creating the orphaned bump commit when TAG
(harness/v${HARNESS_VERSION}) already exists.
Comment on lines
+12
to
+15
| - approval-gate | ||
| - auth-credentials | ||
| - harness | ||
| - hook-fanout |
There was a problem hiding this comment.
Remove harness from the generic tag workflow.
Selecting harness here only bumps the harness manifest, but harness/v* now fans out through release-harness-bundle.yml. That bypasses the new dependency pre-bump logic in create-tag-harness-bundle.yml, so changed in-repo deps can be silently skipped from the bundle publish.
🤖 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/workflows/create-tag.yml around lines 12 - 15, The generic tag
workflow currently includes the list item 'harness' which must be removed so
harness tags are handled only by release-harness-bundle.yml; open the step that
lists items including 'approval-gate', 'auth-credentials', 'harness',
'hook-fanout' and delete the 'harness' entry so create-tag-harness-bundle.yml
and release-harness-bundle.yml can enforce the dependency pre-bump logic
correctly.
Comment on lines
+44
to
+55
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
| fetch-tags: true | ||
|
|
||
| - name: Refetch annotated tag | ||
| env: | ||
| RAW_TAG: ${{ inputs.tag || github.ref_name }} | ||
| run: | | ||
| set -euo pipefail | ||
| git fetch origin "+refs/tags/${RAW_TAG}:refs/tags/${RAW_TAG}" || true | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/release-harness-bundle.ymlRepository: iii-hq/workers
Length of output: 9957
Pin checkout to requested harness tag to prevent code-metadata mismatch.
The initial checkout step doesn't use the inputs.tag input, so the workspace may be on a different commit than the tag whose metadata is being parsed. When this workflow is triggered by push to a tag, the checkout uses the default branch instead of the tag. When triggered manually with a specific tag, checkout again defaults to the branch. The later metadata parsing and build steps then operate on mismatched code, potentially building and publishing a different revision than intended.
Suggested fix
- uses: actions/checkout@v4
with:
fetch-depth: 0
fetch-tags: true
+ ref: ${{ inputs.tag || github.ref_name }}🤖 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/workflows/release-harness-bundle.yml around lines 44 - 55, The
checkout step is not pinned to the intended tag, causing the workspace to
potentially be on a different commit than the tag parsed later; update the
initial actions/checkout@v4 step to include a ref that uses the same tag
expression as RAW_TAG (e.g. ref: ${{ inputs.tag || github.ref_name }}) so the
checked-out commit matches the tag, keep fetch-depth: 0 and fetch-tags: true,
and remove or keep the subsequent "Refetch annotated tag" step as optional;
ensure the RAW_TAG expression used in the git fetch matches the ref used for
actions/checkout to avoid metadata/code mismatches.
| | Function | Role | | ||
| |---|---| | ||
| | `policy::approval_gate` | Subscriber body + `durable:subscriber` trigger on `topic`. | | ||
| | `approval::resolve` | Operator decision (`allow` / `deny`) for one pending `(session_id, tool_call)`. | |
There was a problem hiding this comment.
Align identifier terminology with function_call_id to avoid API confusion.
At Line 88, (session_id, tool_call) is inconsistent with the rest of the docs that use function_call_id (with tool_call_id only as a legacy alias). Recommend changing this row to (session_id, function_call_id) for consistency.
🤖 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 `@approval-gate/README.md` at line 88, Update the approval::resolve
documentation row to use the consistent identifier name: replace the tuple
description "(session_id, tool_call)" with "(session_id, function_call_id)"
(keeping a note that tool_call_id is a legacy alias only if needed); ensure any
adjacent mentions in the same table row or nearby text reference session_id and
function_call_id so the README uses function_call_id everywhere consistent with
the rest of the docs.
Comment on lines
+78
to
+80
| ### 3. The 14 expected workers | ||
|
|
||
| `EXPECTED_WORKERS` (`lib.rs:18`) is the source of truth for what the harness assumes is on the bus. Grouped by role: | ||
| `EXPECTED_WORKERS` (generated by `build.rs` from `iii.worker.yaml`, included into `lib.rs`) is the source of truth for what the harness assumes is on the bus. Grouped by role: |
There was a problem hiding this comment.
Worker count is inconsistent within the document.
This section still references 14 workers, while the intro states 13. Please align the heading and the sample payload count so readers don’t get conflicting bundle expectations.
Also applies to: 160-160
🤖 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 `@harness/ARCHITECTURE.md` around lines 78 - 80, The document header "The 14
expected workers" and any sample payload counts referencing 14 must be
reconciled with the intro that says 13; update the heading and all sample
payload counts to match the actual EXPECTED_WORKERS value generated from
iii.worker.yaml (and included into lib.rs via build.rs) so the document
consistently reflects the real worker count (13) — search for
"EXPECTED_WORKERS", the heading text, and any sample payload blocks and change
"14" to the correct number.
Comment on lines
+31
to
+37
| .skip_while(|l| !l.starts_with("dependencies:")) | ||
| .skip(1) | ||
| .take_while(|l| l.is_empty() || l.starts_with(' ') || l.starts_with('\t')) | ||
| .filter(|l| l.starts_with(" ") && l.contains(':')) | ||
| .filter_map(|l| l.split(':').next()) | ||
| .map(|name| name.trim().to_string()) | ||
| .filter(|name| !name.is_empty()) |
There was a problem hiding this comment.
Worker extraction is too permissive and can parse nested YAML keys as worker names.
With the current filters, nested lines like version: can be interpreted as workers. Restrict parsing to exactly top-level dependency entries under dependencies: (e.g., exact two-space indentation + valid worker-name key pattern).
Suggested hardening
- .filter(|l| l.starts_with(" ") && l.contains(':'))
- .filter_map(|l| l.split(':').next())
- .map(|name| name.trim().to_string())
+ .filter_map(|l| {
+ let rest = l.strip_prefix(" ")?;
+ if rest.starts_with(' ') || rest.starts_with('\t') {
+ return None; // nested key, not a dependency name
+ }
+ let (name, _) = rest.split_once(':')?;
+ let name = name.trim();
+ if name.is_empty() {
+ return None;
+ }
+ if name
+ .chars()
+ .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_')
+ {
+ Some(name.to_string())
+ } else {
+ None
+ }
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .skip_while(|l| !l.starts_with("dependencies:")) | |
| .skip(1) | |
| .take_while(|l| l.is_empty() || l.starts_with(' ') || l.starts_with('\t')) | |
| .filter(|l| l.starts_with(" ") && l.contains(':')) | |
| .filter_map(|l| l.split(':').next()) | |
| .map(|name| name.trim().to_string()) | |
| .filter(|name| !name.is_empty()) | |
| .skip_while(|l| !l.starts_with("dependencies:")) | |
| .skip(1) | |
| .take_while(|l| l.is_empty() || l.starts_with(' ') || l.starts_with('\t')) | |
| .filter_map(|l| { | |
| let rest = l.strip_prefix(" ")?; | |
| if rest.starts_with(' ') || rest.starts_with('\t') { | |
| return None; // nested key, not a dependency name | |
| } | |
| let (name, _) = rest.split_once(':')?; | |
| let name = name.trim(); | |
| if name.is_empty() { | |
| return None; | |
| } | |
| if name | |
| .chars() | |
| .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') | |
| { | |
| Some(name.to_string()) | |
| } else { | |
| None | |
| } | |
| }) | |
| .filter(|name| !name.is_empty()) |
🤖 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 `@harness/build.rs` around lines 31 - 37, The worker extraction is too
permissive (current chain using skip_while/take_while/filter/filter_map/map) and
treats nested YAML keys like " version:" as worker names; tighten the filter
that currently checks starts_with(" ") && contains(':') to require an exact
top-level dependency key pattern — either replace that closure with a regex
match like r"^ [A-Za-z0-9_-]+:" or explicitly check that the line starts with
exactly two spaces followed immediately by a valid key (alphanumeric, dash or
underscore) and a colon (e.g., ensure line.len() > 2, &line[0..2] == " ", then
validate the key portion before the colon), so only true top-level dependency
entries under "dependencies:" are captured (adjust the .filter and/or
.filter_map around the current filter_map(|l| l.split(':').next()) accordingly).
Comment on lines
+123
to
+131
| #[test] | ||
| fn done_is_terminal() { | ||
| let ev = AssistantMessageEvent::Stop { | ||
| stop_reason: StopReason::End, | ||
| error_message: None, | ||
| error_kind: None, | ||
| }; | ||
| assert!(!ev.is_terminal()); | ||
| } |
There was a problem hiding this comment.
Misnamed test; no positive coverage for is_terminal true case.
done_is_terminal actually constructs a Stop variant and asserts !is_terminal(), i.e. it verifies "Stop is non-terminal". The Done/Error → true path is never exercised.
🧪 Proposed fix
- #[test]
- fn done_is_terminal() {
- let ev = AssistantMessageEvent::Stop {
- stop_reason: StopReason::End,
- error_message: None,
- error_kind: None,
- };
- assert!(!ev.is_terminal());
- }
+ #[test]
+ fn stop_is_not_terminal() {
+ let ev = AssistantMessageEvent::Stop {
+ stop_reason: StopReason::End,
+ error_message: None,
+ error_kind: None,
+ };
+ assert!(!ev.is_terminal());
+ }
+
+ #[test]
+ fn done_and_error_are_terminal() {
+ let msg = AssistantMessage::default();
+ assert!(AssistantMessageEvent::Done { message: msg.clone() }.is_terminal());
+ assert!(AssistantMessageEvent::Error { error: msg }.is_terminal());
+ }(Adjust AssistantMessage::default() to whatever construction is available in this crate.)
🤖 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 `@harness/crates/harness-types/src/stream_event.rs` around lines 123 - 131, The
test named done_is_terminal is misnamed and only asserts that
AssistantMessageEvent::Stop { stop_reason: StopReason::End, ... } returns false
from is_terminal(), leaving the true path untested; update the tests to (1)
rename the existing test to reflect that Stop is non-terminal (e.g.,
stop_is_non_terminal) and (2) add a new test that constructs a terminal event
(for example using AssistantMessageEvent::Done or an AssistantMessage::default()
converted into the terminal variant, or constructing the variant that represents
Done/Error) and asserts is_terminal() returns true; reference
AssistantMessageEvent::Stop, StopReason::End, is_terminal and the terminal
variant (Done/Error/AssistantMessage::default) when making the changes.
clippy --all-targets caught: - harness-types::ThinkingLevel had a manual Default impl that clippy::derivable_impls flags; switched to derive(Default) + #[default] on Off. - tests/phase_a.rs imported turn_orchestrator::persistence directly, but the prior commit removed turn-orchestrator from harness dev-dependencies. Delete the test (the workflow it covers no longer matches the trimmed dev-dep set; resurrect via a process-level test if needed).
This was referenced May 11, 2026
Merged
This was referenced Jun 12, 2026
Merged
This was referenced Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
release-harness-bundle.yml: oneharness/v*tag fans out builds + publishes for harness and every in-repo dep whose local manifest version is ahead of the registry. Reuses_rust-binary.ymland_publish-registry.ymlexactly as the per-workerrelease.ymldoes.create-tag-harness-bundle.yml: oneworkflow_dispatchbumps harness, pre-bumps deps with source changes since their last tag, and pushes the umbrellaharness/v<X.Y.Z>annotated tag.release.ymldropsharness/v*from its tag patterns so the bundle workflow owns harness; everything else still uses the per-worker path.approval-gate,provider-anthropic,provider-openaito0.2.0(substantive src changes since0.1.0).pr-checksmetadata_globsto includeCargo.tomlso description-only edits don't force a version bump (still strict onsrc/,build.rs,tests/, etc.).harness.default_config.workersfromregistry/index.json— the harness binary'sWorkerConfigonly deserializesengine_url, so the array was dead config.build.rs,src/{config,manifest}.rs,skill.md,skills/,tests/{manifest,skill}.rs) and the newharness/crates/harness-typessubcrate that the prior commit missed..gitignore: ignoreharness/data/(sled state).Test plan
pr-checksmatrix passes for every changed worker (theCargo.tomlallowlist relaxation should keep cosmetic-only deps out of strict mode).Create harness bundle tagfrom the Actions tab on this branch withbump_harness=patchand confirm the script computes a sensible publish set without pushing.Create harness bundle tagwithbump_harness=minor,registry_tag=latest. Confirm the resultingrelease-harness-bundle.ymlrun builds 4 workers (harness + the 3 pre-bumped) and POSTs each tohttps://api.workers.iii.dev/publishwith 2xx.iii worker add harnessin a clean dir resolves harness0.3.0, fetches all deps at registered versions, andiii startboots cleanly.Out-of-scope this PR (stashed locally for a follow-up):
acp,iii-database,iii-lsp,image-resize,mcp,storage,skills.Summary by CodeRabbit
New Features
Chores
Documentation