Conversation
📝 WalkthroughWalkthroughOpenCode agent links now support optional provider pins and per-provider profile generation. Status validation and CLI layout handling reflect these profiles. Successful unlink operations archive manifests in inactive paths. ChangesAgent link provider materialization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant AgentCLI
participant Reconcile
participant ProviderRoutes
participant OpenCode
AgentCLI->>Reconcile: create link with optional provider
Reconcile->>ProviderRoutes: resolve enabled providers
ProviderRoutes->>OpenCode: write profiles and publish routes
OpenCode-->>AgentCLI: expose provider-specific layout
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/agent-migration/src/reconcile.rs (1)
876-904: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep manifest archiving resumable after partial archive failure.
manifest_chainstarts from the selected path and followsprevious_manifest, sochain.first()is the latest manifest. The final loops first writeunlinked = truefor every manifest, then rename every manifest in latest-first order. Ifmanifest::archive_manifestfails after the latest manifest is renamed, a retry re-reads either the archived latest or anotherunlinkedmanifest and hitsbail!("manifest ... has already been unlinked"). Pair theunlinkedwrite with that manifest’s archive result, or defer marking until the archive step succeeds.🤖 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 `@crates/agent-migration/src/reconcile.rs` around lines 876 - 904, Make manifest archiving resumable in the unlink flow around manifest_chain and archive_manifest: do not mark every manifest unlinked before any rename succeeds. Process each manifest in latest-first order, archive it first, then persist unlinked = true for that same manifest, while ensuring retries recognize already archived manifests and continue with remaining ancestors without triggering the initial already-unlinked bail.
🧹 Nitpick comments (1)
crates/gateway-cli/src/cli/agent.rs (1)
319-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit "all effective providers" indicator for unpinned verbatim main-account links.
For an unpinned main-account
switch/passthroughbinding,binding.providerisNone, so theprovider:line at line 329-331 is skipped. Theprovider_filter:line at line 332-342 is also skipped becausemode.is_verbatim()is true. The result:agent show/agent listoutput showsprofile_layout: PerProviderbut nothing tells the user this means every enabled provider is linked, unlike theprovider_filtercase which explicitly prints "(all effective providers)" when unset.Print an explicit line for this case to match the clarity already given to
provider_filter.♻️ Proposed clarity improvement
if let Some(provider) = binding.provider.as_deref() { println!(" provider: {provider}"); + } else if binding.account_source == AgentAccountSource::Main && binding.mode.is_verbatim() { + println!(" provider: (all effective providers)"); }🤖 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 `@crates/gateway-cli/src/cli/agent.rs` around lines 319 - 357, Update the binding display in the status match to explicitly indicate “all effective providers” when AgentAccountSource::Main uses a verbatim mode and binding.provider is unset. Add this indicator alongside the existing provider/provider_filter output, while preserving current output for pinned providers and non-verbatim bindings.
🤖 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 `@crates/agent-migration/src/manifest.rs`:
- Around line 76-90: Update archive_manifest to perform the
destination-existence check atomically with the rename, using no-replace rename
semantics such as renameat2 with RENAME_NOREPLACE or an equivalent mechanism.
Remove reliance on the separate archived.exists() check so a concurrently
created destination cannot be overwritten, while preserving the existing error
context and return behavior.
- Around line 83-90: Update the unlink_inner/archive_manifest flow so archiving
cannot leave earlier marked manifests undiscoverable when a later archive fails:
preflight all archive destinations before moving any manifest, or implement
rollback/idempotent recovery for partial failures. Preserve the existing
unlinked marking while ensuring retries can still discover and recover every
source manifest.
---
Outside diff comments:
In `@crates/agent-migration/src/reconcile.rs`:
- Around line 876-904: Make manifest archiving resumable in the unlink flow
around manifest_chain and archive_manifest: do not mark every manifest unlinked
before any rename succeeds. Process each manifest in latest-first order, archive
it first, then persist unlinked = true for that same manifest, while ensuring
retries recognize already archived manifests and continue with remaining
ancestors without triggering the initial already-unlinked bail.
---
Nitpick comments:
In `@crates/gateway-cli/src/cli/agent.rs`:
- Around line 319-357: Update the binding display in the status match to
explicitly indicate “all effective providers” when AgentAccountSource::Main uses
a verbatim mode and binding.provider is unset. Add this indicator alongside the
existing provider/provider_filter output, while preserving current output for
pinned providers and non-verbatim bindings.
🪄 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 Plus
Run ID: 833ac553-8fd1-4218-aa6f-5f02ef13a95b
📒 Files selected for processing (7)
README.mdcrates/agent-migration/src/manifest.rscrates/agent-migration/src/reconcile.rscrates/agent-migration/src/status.rscrates/config/src/lib.rscrates/gateway-cli/src/cli/agent.rscrates/router/src/api/mod.rs
| if archived.exists() { | ||
| bail!( | ||
| "cannot archive migration manifest {} because {} already exists", | ||
| path.display(), | ||
| archived.display() | ||
| ); | ||
| } | ||
| std::fs::rename(path, &archived).with_context(|| { | ||
| format!( | ||
| "archiving migration manifest {} to {}", | ||
| path.display(), | ||
| archived.display() | ||
| ) | ||
| })?; | ||
| Ok(archived) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Expect: every archive destination is protected by the same lock,
# or archive_manifest uses a no-replace operation.
rg -n -C 12 \
'\barchive_manifest\s*\(|\btry_lock_agent_in\s*\(|\bmanifest_chain\s*\(' \
crates/agent-migration/src/manifest.rs \
crates/agent-migration/src/reconcile.rsRepository: agentic-rs/tokn
Length of output: 14698
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect manifest path helpers and archive implementations, plus lock placement around callers.
sed -n '68,130p' crates/agent-migration/src/manifest.rs
printf '\n--- reconcile archive call site ---\n'
sed -n '863,910p' crates/agent-migration/src/reconcile.rs
printf '\n--- inactive_manifest_path usages ---\n'
rg -n "inactive_manifest_path" crates/agent-migration/srcRepository: agentic-rs/tokn
Length of output: 6275
Make the archive destination check atomic.
archive_manifest checks archived.exists() before calling std::fs::rename, so a destination created after the check can be overwritten. Use atomic no-replace rename semantics such as renameat2(..., RENAME_NOREPLACE), or ensure the same lock covers every possible archive destination.
🤖 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 `@crates/agent-migration/src/manifest.rs` around lines 76 - 90, Update
archive_manifest to perform the destination-existence check atomically with the
rename, using no-replace rename semantics such as renameat2 with
RENAME_NOREPLACE or an equivalent mechanism. Remove reliance on the separate
archived.exists() check so a concurrently created destination cannot be
overwritten, while preserving the existing error context and return behavior.
| std::fs::rename(path, &archived).with_context(|| { | ||
| format!( | ||
| "archiving migration manifest {} to {}", | ||
| path.display(), | ||
| archived.display() | ||
| ) | ||
| })?; | ||
| Ok(archived) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'crates/agent-migration/src/manifest.rs|agent-migration' || true
echo "== outline manifest =="
ast-grep outline crates/agent-migration/src/manifest.rs --view expanded || true
echo "== relevant manifest lines =="
wc -l crates/agent-migration/src/manifest.rs
cat -n crates/agent-migration/src/manifest.rs | sed -n '1,160p'
echo "== search related symbols =="
rg -n "fn archive_manifest|archiving migration manifest|fn unlink_inner|unlinked|active|discovery|migration manifest|archived" crates/agent-migration/src -SRepository: agentic-rs/tokn
Length of output: 28177
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== reconcile manifest archive lines =="
sed -n '863,908p' crates/agent-migration/src/reconcile.rs | cat -n
echo "== success tests around multi-manifest cleanup =="
sed -n '5135,5200p' crates/agent-migration/src/reconcile.rs | cat -n
sed -n '5848,5900p' crates/agent-migration/src/reconcile.rs | cat -n
sed -n '6662,6698p' crates/agent-migration/src/reconcile.rs | cat -n
echo "== test archive failures / coverage search =="
rg -n "archive|unlink_inner|chain|previous_manifest|unlinked|inactive" crates/agent-migration/src/reconcile.rs crates/agent-migration/src -S | rg -n "test|#\\[test\\]|fn .*archive|archive_manifest|rename|rename_failed|archiving|chain::" || true
echo "== static diff =="
git diff -- crates/agent-migration/src/manifest.rs crates/agent-migration/src/reconcile.rs | sed -n '1,240p'Repository: agentic-rs/tokn
Length of output: 11702
Preserve chain manifests when archiving fails after marking.
unlink_inner writes unlinked = true for every manifest before archiving any of them. If a later archive_manifest call fails, earlier manifests have been moved out of the active namespace while their unlinked state persists, and normal retry cannot discover those source manifests. Preflight archive paths or add rollback/idempotent recovery for partial archive failure.
🤖 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 `@crates/agent-migration/src/manifest.rs` around lines 83 - 90, Update the
unlink_inner/archive_manifest flow so archiving cannot leave earlier marked
manifests undiscoverable when a later archive fails: preflight all archive
destinations before moving any manifest, or implement rollback/idempotent
recovery for partial failures. Preserve the existing unlinked marking while
ensuring retries can still discover and recover every source manifest.
1 participant
Summary
switchandpassthroughlinks publish every enabled provider unless--providerexplicitly pins one./opencode-<provider>/v1profile, pinned to that provider, so it executes the requested verbatim mode instead of falling back toroute.tokn-router-deepseeknamespace.Validation
cargo test --locked -p tokn-agent-migrationcargo test --locked -p tokn-routercargo test --locked -p tokn-gateway-cli --test e2e_proxycargo clippy --locked --workspace --all-targets --all-features -- -D warningscargo build --locked --workspace --all-featurescargo test --locked --workspace --all-featuresadditionally reaches an unrelated pre-existing Z.ai price-ratio assertion that reads the local runtime catalogue cache; the changed packages pass independently.Summary by CodeRabbit
New Features
--providerto restrict a link to one provider; multi-provider selection remains available through provider filters.Bug Fixes
Documentation