feat(traces): Trace Commons instance enrollment CLI + hosted-user account login links - #5858
Conversation
… enrollment CLI Closes the Path B gap from PR #5280: onboard_instance_with_sink existed but had no product entry point, so invite-based (DeviceKey, per-user-attributed) instance enrollment was unreachable for admins. - onboarding::onboard_instance_at_base — base-dir-parameterised instance enrollment with the default direct sink; targets the scope-None location so every user without a personal enrollment inherits it via resolve_trace_credentials. Test pins the instance-dir targeting (policy + promoted device key at trace_contributions/, no users/<hash> dir). - New CLI subcommand: traces enroll-instance --invite <url> [--include-message-text] [--include-tool-payloads] [--json]. Host-shell possession is the admin gate, matching traces opt-in's trust boundary for the global policy. Parse tests cover flags and required --invite. - slice1 plan note updated: the deferred admin surface now exists. Also in this commit (same file): mint_account_login_link direct variant and DirectPinnedContributionSink used by the WebUI surface in the next commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…login links
Hosted multi-tenant users have no host shell, and the agent capability
delivers the one-time login URL to a local file they cannot read. This adds
the product path: an authenticated WebUI action that mints the link and
returns it directly to the caller's browser — the URL never touches a local
file, a log line, or any model-visible surface.
- product_workflow: account_login_link_for_user +
RebornServicesApi::trace_account_login_link (caller-derived scope;
unenrolled → zero-state, not an error).
- webui_v2: POST /api/webchat/v2/traces/account-login-link
(webui.v2.trace_account_login_link), NoBody, per-caller 10/min rate limit
(each call mints a credential). Descriptor + handler contract tests; route
table row in CLAUDE.md.
- frontend: 'Open Trace Commons account' button on the Trace Commons settings
tab. openAccountLoginLink opens a blank tab synchronously (popup-blocker
attribution) with noopener,noreferrer, then navigates it to the minted URL;
closes the placeholder on unenrolled/error. i18n for all 11 languages.
- test-runner fix: trace-commons-tab.test.mjs had silently stopped running
after the frontend migration (vitest include is *.{test,spec}.{ts,tsx});
converted to trace-commons-tab.test.ts with the window.open fake capturing
every argument the production caller passes. Suite: 582 → 590.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
github-actions
Bot
added
scope: docs
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Trace Commons account-login-link minting and WebUI wiring, plus a CLI instance-enrollment command backed by a new onboarding entrypoint. ChangesTrace Commons account login link
CLI instance enrollment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WebUiV2Handler
participant RebornServicesApi
participant trace_credits
participant DirectPinnedContributionSink
participant Issuer
WebUiV2Handler->>RebornServicesApi: trace_account_login_link(caller)
RebornServicesApi->>trace_credits: account_login_link_for_user(tenant_id, user_id)
trace_credits->>DirectPinnedContributionSink: mint_account_login_link(tenant_id, user_id)
DirectPinnedContributionSink->>Issuer: POST /v1/account/login-links
Issuer-->>DirectPinnedContributionSink: response
DirectPinnedContributionSink-->>trace_credits: AccountLoginLink or error
trace_credits-->>RebornServicesApi: RebornAccountLoginLinkResponse
sequenceDiagram
participant TraceCommonsTab
participant openAccountLoginLink
participant window
participant mintAccountLoginLink
participant WebUIBackend
TraceCommonsTab->>openAccountLoginLink: handleOpenAccount()
openAccountLoginLink->>window: open(about:blank, _blank)
openAccountLoginLink->>mintAccountLoginLink: mint()
mintAccountLoginLink->>WebUIBackend: POST /api/webchat/v2/traces/account-login-link
WebUIBackend-->>mintAccountLoginLink: {minted, enrolled, url}
mintAccountLoginLink-->>openAccountLoginLink: response
alt minted with url
openAccountLoginLink->>window: navigate to url
else unavailable/blocked/error
openAccountLoginLink->>window: close placeholder
end
openAccountLoginLink-->>TraceCommonsTab: status
sequenceDiagram
participant TracesCLI
participant contributor
participant onboarding
participant Issuer
TracesCLI->>contributor: dispatch EnrollInstance
contributor->>onboarding: onboard_instance_at_base(base_dir, invite_url, consents)
onboarding->>Issuer: POST /v1/onboard
Issuer-->>onboarding: onboarding response
onboarding-->>contributor: OnboardOutcome
contributor-->>TracesCLI: printed enrollment result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 2 | 0 | 2 | 1b94f157b092 |
Head: 1b94f157b0926987878a9530e29427743d2645b9
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found two blocking issues in the new Trace Commons account-opening flow: the browser tab handle is lost when using noopener/noreferrer, and relative login URLs are not made absolute before navigation.
Findings
Blocking: 2 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] noopener makes the opened tab unreachable
Location: crates/ironclaw_webui_v2/frontend/src/pages/settings/components/trace-commons-tab.tsx:33
window.open(..., "noopener,noreferrer") returns null in modern browsers even when the popup is allowed. That means the click still mints a one-time login URL, but line 40 treats the handle as blocked and never navigates the tab, so the new Open Account button fails and consumes the credential. Open the placeholder tab in a way that keeps a handle long enough to set location (for example, clear opener immediately after opening), or change the flow so it does not need to navigate a noopener window after the async mint.
2. ❌ [MEDIUM] Relative login URLs navigate to the IronClaw origin
Location: crates/ironclaw_webui_v2/frontend/src/pages/settings/components/trace-commons-tab.tsx:41
The backend contract currently accepts and tests login-link responses like /account/login?code=abc, but assigning that string to the blank tab's location resolves it relative to the WebUI/IronClaw origin, not the Trace Commons issuer. In that case users land on /account/login on the local app and the one-time Trace Commons code is unusable. Normalize the minted URL to an absolute issuer URL before returning it to the browser, or otherwise resolve relative URLs against the Trace Commons account API origin before navigation.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/stale/stalled state while reviewers run.
| // user's click, then navigates it to the minted URL. The URL exists only in | ||
| // this flow — never logged, never stored. | ||
| export async function openAccountLoginLink({ mint, open }) { | ||
| const win = open("about:blank", "_blank", "noopener,noreferrer"); |
There was a problem hiding this comment.
noopener,noreferrer causes window.open to return null in modern browsers, so the async mint succeeds but the code reports blocked and never navigates the tab. Keep a usable handle until location is set, or avoid post-mint navigation of a noopener window.
There was a problem hiding this comment.
Fixed in ee60b3a. You're right — with noopener in the features string, window.open returns null and the flow could never navigate the tab. The helper now opens about:blank without features and severs win.opener = null manually (same reverse-tabnabbing protection, handle retained). The test fake now models the real browser contract — a noopener feature yields a null handle — so a regression reintroducing the feature string fails the suite.
| return { status: "unavailable" }; | ||
| } | ||
| if (!win) return { status: "blocked" }; | ||
| win.location = response.url; |
There was a problem hiding this comment.
This assumes response.url is absolute. The backend/tests allow /account/login?code=..., which resolves against the WebUI origin here instead of the Trace Commons issuer. Normalize relative login-link URLs to an absolute issuer URL before navigating.
There was a problem hiding this comment.
Fixed in ee60b3a. Correct — the server can return a relative path and navigating the tab to it would resolve against the IronClaw origin. The fix is crate-side in mint_account_login_link_inner: a relative url in the response is resolved against the login-links endpoint (whose origin is the trust-anchored issuer origin) before being returned, so every delivery channel — WebUI navigation, agent-path local delivery file — receives an absolute URL. Tests on both the sink and direct paths now assert the absolutized form.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b94f157b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // user's click, then navigates it to the minted URL. The URL exists only in | ||
| // this flow — never logged, never stored. | ||
| export async function openAccountLoginLink({ mint, open }) { | ||
| const win = open("about:blank", "_blank", "noopener,noreferrer"); |
There was a problem hiding this comment.
With noopener,noreferrer in the windowFeatures argument, browsers return null even when the popup was successfully opened. In the normal click path that makes win null, so after the async mint succeeds this flow reports blocked and never navigates the already-open blank tab; the one-time login URL is discarded. Please keep a navigable handle, or use a different safe flow, before adding noopener protections.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ee60b3a — see the reply to the ironloop thread on this line: the noopener feature is no longer passed (null-handle problem); win.opener is severed manually and the test fake models the null-on-noopener browser contract.
| return { status: "unavailable" }; | ||
| } | ||
| if (!win) return { status: "blocked" }; | ||
| win.location = response.url; |
There was a problem hiding this comment.
Trace Commons responses are allowed to return relative login paths: the trace client stores parsed["url"] verbatim in mint_account_login_link_inner, and AccountLoginLink::url documents /account/login?code=… as typical. Assigning such a value here resolves it against the WebUI origin, so users are sent to IronClaw's /account/login instead of the Trace Commons host and the single-use code is stranded. Resolve relative links against the issuer origin or require an absolute URL before returning/opening it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ee60b3a — relative response URLs are now absolutized crate-side against the trust-anchored issuer origin (mint_account_login_link_inner), covering both the WebUI and agent delivery channels, with test assertions on both paths.
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.15% — 282832 / 332166 lines Per-crate breakdown (65 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (4 entry/entries excluded from the accounting above)
|
![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
🤖 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/ironclaw_reborn_traces/src/contribution.rs`:
- Around line 5557-5635: The direct login-link POST path in
DirectPinnedContributionSink can send a bearer token to an unvetted URL, so
re-run issuer validation before this call or restrict the sink to trusted
callers only. Update the caller in mint_account_login_link_direct to validate
the derived account_api_base_url using the same URL checks used elsewhere in
this module, or make DirectPinnedContributionSink private so only already-vetted
code can invoke execute.
In
`@crates/ironclaw_webui_v2/frontend/src/pages/settings/components/trace-commons-tab.tsx`:
- Around line 32-46: The popup-blocked check in openAccountLoginLink is
happening too late, causing mint() to burn a one-time login URL even when
window.open returns null. Move the !win guard immediately after the open() call
and before awaiting mint(), so the function returns { status: "blocked" }
without minting when the popup is blocked. Keep the rest of the
openAccountLoginLink flow unchanged, including the existing success and error
handling.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 188a4da4-01e7-4586-8b3d-9ec43284496c
📒 Files selected for processing (32)
crates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/reborn_services/trace_credits.rscrates/ironclaw_reborn_cli/src/commands/traces/contributor.rscrates/ironclaw_reborn_cli/src/commands/traces/mod.rscrates/ironclaw_reborn_cli/src/commands/traces/tests.rscrates/ironclaw_reborn_traces/src/contribution.rscrates/ironclaw_reborn_traces/src/onboarding/mod.rscrates/ironclaw_reborn_traces/src/onboarding/tests.rscrates/ironclaw_webui_v2/CLAUDE.mdcrates/ironclaw_webui_v2/frontend/src/i18n/ar.tscrates/ironclaw_webui_v2/frontend/src/i18n/de.tscrates/ironclaw_webui_v2/frontend/src/i18n/en.tscrates/ironclaw_webui_v2/frontend/src/i18n/es.tscrates/ironclaw_webui_v2/frontend/src/i18n/fr.tscrates/ironclaw_webui_v2/frontend/src/i18n/hi.tscrates/ironclaw_webui_v2/frontend/src/i18n/ja.tscrates/ironclaw_webui_v2/frontend/src/i18n/ko.tscrates/ironclaw_webui_v2/frontend/src/i18n/pt-BR.tscrates/ironclaw_webui_v2/frontend/src/i18n/uk.tscrates/ironclaw_webui_v2/frontend/src/i18n/zh-CN.tscrates/ironclaw_webui_v2/frontend/src/pages/settings/components/trace-commons-tab.test.mjscrates/ironclaw_webui_v2/frontend/src/pages/settings/components/trace-commons-tab.test.tscrates/ironclaw_webui_v2/frontend/src/pages/settings/components/trace-commons-tab.tsxcrates/ironclaw_webui_v2/frontend/src/pages/settings/lib/settings-api.tscrates/ironclaw_webui_v2/src/descriptors.rscrates/ironclaw_webui_v2/src/handlers.rscrates/ironclaw_webui_v2/src/lib.rscrates/ironclaw_webui_v2/src/router.rscrates/ironclaw_webui_v2/tests/webui_v2_descriptors_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rsdocs/superpowers/plans/2026-06-25-trace-commons-slice1-resolver-instance-enrollment.md
💤 Files with no reviewable changes (1)
- crates/ironclaw_webui_v2/frontend/src/pages/settings/components/trace-commons-tab.test.mjs
…dle, mint after popup check - mint_account_login_link_inner resolves a relative response url against the login-links endpoint (trust-anchored issuer origin); a relative url would otherwise resolve against the consuming surface's own origin (e.g. the IronClaw WebUI host). Tests assert the absolutized form on both paths. - openAccountLoginLink no longer passes the noopener feature to window.open — modern browsers return null for noopener opens, which made navigation impossible; the handle's opener is severed manually instead (reverse-tabnabbing protection preserved). The test fake now models the real browser contract (noopener feature → null handle). - Popup-blocked opens short-circuit BEFORE the mint call so a blocked tab no longer burns a single-use login link server-side; test asserts zero mints. - DirectPinnedContributionSink is pub(crate) with a documented invariant: request URLs must come from the enrolled policy's trust-anchored endpoint derivations, never caller input. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@IronLoop review |
|
@IronLoop review --agent reviewer |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee60b3a5a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| win.close(); | ||
| return { status: "unavailable" }; | ||
| } | ||
| win.location = response.url; |
There was a problem hiding this comment.
When the Trace Commons login-link response contains a non-http(s) URL such as javascript:... (the backend currently accepts any absolute url string), this assignment navigates the newly opened about:blank tab directly to it. That tab inherits the WebUI origin, so a javascript URL can execute with access to WebUI-origin storage such as sessionStorage["ironclaw_token"] even though opener was nulled. Validate that the minted URL is http:/https: (and preferably an expected Trace Commons origin) before assigning it, or reject invalid URLs server-side.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f911ae9 + 3548a (two layers, both regression-tested):
Server-side (primary): mint_account_login_link_inner now pins the final resolved URL to the trust-anchored issuer origin — scheme + host + port must match the login-links endpoint, userinfo rejected — so a hostile/compromised issuer response can no longer steer the navigation anywhere (this excludes javascript:/data: by construction). Tests cover cross-origin absolute, javascript:, and userinfo rejection.
Client-side (defense in depth, this thread's suggestion): isSafeLoginLinkUrl refuses to navigate to anything that is not absolute http(s) before the win.location assignment, independently of the backend guarantee — with a test asserting the tab is closed and never navigated for a javascript: mint.
… transport tests, UI guards - SECURITY: mint_account_login_link_inner now rejects any response url whose final resolved form leaves the trust-anchored issuer origin (scheme + host + port must match the login-links endpoint; userinfo rejected) — a hostile or compromised issuer response could otherwise steer the authenticated browser popup to an arbitrary origin or a javascript: URL. Tests cover relative absolutization, cross-origin rejection, javascript: rejection, and userinfo rejection. - DirectPinnedContributionSink transport hardening is now regression-tested: link-local/metadata hosts rejected at resolution, 3xx surfaced without contacting the Location target, oversized bodies rejected during the streamed read. - Frontend: handleOpenAccount gains an in-flight ref (double-click burns a one-time link) and a mounted ref (no setState after unmount). - Handler contract test now asserts the forwarded caller carries the authenticated tenant AND user id, not just call count. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The about:blank tab inherits the WebUI origin, so a javascript: URL would execute with WebUI-origin access (e.g. session storage). The backend now origin-pins the minted URL; the client additionally refuses to navigate to anything that is not absolute http(s), independently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Local Codex (GPT) advisory review round complete — verdict was REQUEST CHANGES with one Critical, all findings addressed in f911ae9 and c6466d6:
Suites green: reborn_traces 216, webui_v2 contracts, frontend 591 + tsc, clippy clean. |
|
@IronLoop review --agent reviewer |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6466d662a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "Users can exclude themselves with `ironclaw-reborn traces opt-out`; an explicit \ | ||
| opt-out always wins over instance enrollment." |
There was a problem hiding this comment.
After enroll-instance, the instance enrollment lives in the root trace_contributions/policy.json, but opt_out always flips that same root policy to enabled = false before writing the scoped disabled policy. In an instance-enrolled deployment, following this new guidance (even with --user-scope) disables the inherited enrollment for everyone, not just the one user who wants to opt out; this needs a scoped-only opt-out path or different guidance.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b056928 — great catch, this was the most consequential finding of the round. traces opt-out (any form) flipped the root policy first, and since #5280 that file IS the instance enrollment, so opting out one user silently disenrolled the whole instance.
Split the semantics:
--user-scope <tenant>/<user>now performs a scoped-ONLY opt-out via a new crate primitive (opt_out_user_scope) — the instance policy is untouched; the resolver's explicit-opt-out precedence handles the rest. Regression test pins all three properties: instance policy still enabled on disk, the opted-out user stops resolving, other users keep inheriting.- Bare
opt-outkeeps the legacy full-disable (global/instance + owner scope) as the deliberate off switch, and now prints a note pointing at--user-scopefor single-user opt-outs. - The
enroll-instanceguidance text names the scoped form and warns about the bare form.
…ne user Codex review on c6466d6 caught that 'traces opt-out' always flips the ROOT policy before the scoped one — and since #5280 that root file IS the instance-wide enrollment, so opting out one user (even with --user-scope) silently disenrolled the entire instance. The enroll-instance guidance text pointed users at exactly that command. - New primitive ironclaw_reborn_traces::contribution::opt_out_user_scope[_at]: writes ONLY the scoped policy with enabled=false (the resolver's explicit opt-out signal). Test pins: instance policy untouched on disk, the opted-out user stops resolving, other users keep inheriting. - CLI: --user-scope now performs a scoped-only opt-out; bare opt-out keeps the legacy full-disable semantics and prints a note distinguishing the two. enroll-instance guidance names the scoped form. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@IronLoop review --agent reviewer |
|
@codex review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_cli/src/commands/traces/mod.rs (1)
135-139: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHelp text points users to bare
traces opt-outfor self-exclusion, but bare opt-out now disables the entire instance.Line 139 says "users can exclude themselves with
traces opt-out", yet the reworkedopt_out(lines 554-580) makes baretraces opt-outthe full-instance kill switch. A user following this help text would accidentally disenroll the whole instance instead of opting out just themselves. The runtime output at lines 636-640 correctly directs users to--user-scope, but the clap help text does not.📝 Proposed fix for help text
/// Enroll this ENTIRE INSTANCE in Trace Commons with an operator invite /// link (admin operation — requires shell access to the instance host). /// Every user without a personal enrollment inherits it, attributed via a -/// salted per-user pseudonym; users can exclude themselves with -/// `traces opt-out`. +/// salted per-user pseudonym; users can exclude themselves with +/// `traces opt-out --user-scope <tenant-id>/<user-id>` (bare `traces opt-out` +/// disables the entire instance enrollment).🤖 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/ironclaw_reborn_cli/src/commands/traces/mod.rs` around lines 135 - 139, Update the help text in the traces command documentation so it no longer tells users to self-exclude with bare `traces opt-out`, since `opt_out` now treats that as the instance-wide disable path. In the `traces::mod` comment and any generated clap help tied to the `opt_out` command, point users to the user-scoped option instead, matching the runtime guidance already emitted by the `opt_out` flow. Keep the wording aligned with the actual behavior of `opt_out` and `--user-scope` so the help text and command semantics agree.
🤖 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.
Outside diff comments:
In `@crates/ironclaw_reborn_cli/src/commands/traces/mod.rs`:
- Around line 135-139: Update the help text in the traces command documentation
so it no longer tells users to self-exclude with bare `traces opt-out`, since
`opt_out` now treats that as the instance-wide disable path. In the
`traces::mod` comment and any generated clap help tied to the `opt_out` command,
point users to the user-scoped option instead, matching the runtime guidance
already emitted by the `opt_out` flow. Keep the wording aligned with the actual
behavior of `opt_out` and `--user-scope` so the help text and command semantics
agree.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 69355535-2a07-4a1e-96c0-e2b44cb01bde
📒 Files selected for processing (2)
crates/ironclaw_reborn_cli/src/commands/traces/mod.rscrates/ironclaw_reborn_traces/src/contribution.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b05692801e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| include_tool_payloads, | ||
| }; | ||
| let outcome = ironclaw_reborn_traces::onboarding::onboard_instance_at_base( | ||
| &ironclaw_reborn_traces::paths::ironclaw_base_dir(), |
There was a problem hiding this comment.
When ironclaw-reborn traces enroll-instance is run with IRONCLAW_REBORN_HOME set or the default Reborn home (~/.ironclaw/reborn) differs from the legacy base, this line still writes the policy and device key under ironclaw_base_dir() (IRONCLAW_BASE_DIR/~/.ironclaw). That ignores the Reborn CLI state root and violates the crate contract in crates/ironclaw_reborn_cli/AGENTS.md to use IRONCLAW_REBORN_HOME, so an admin can enroll the wrong/legacy state tree instead of the intended Reborn instance.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified against the runtime and keeping ironclaw_base_dir() deliberately — this one's a false positive at the behavior level. The ENTIRE trace-commons subsystem lives under IRONCLAW_BASE_DIR/~/.ironclaw/trace_contributions: the runtime resolver the serve loop uses (resolve_trace_credentials → ironclaw_common::paths::ironclaw_base_dir()), the capture gate and flush worker in ironclaw_reborn_composition, the existing traces opt-in/opt-out/status/queue commands, and the agent capabilities. If enroll-instance wrote under IRONCLAW_REBORN_HOME instead, the enrollment would land where the resolver never reads and the feature would silently not work. IRONCLAW_REBORN_HOME currently scopes providers.json and extension state, not trace contributions — the AGENTS.md line guards against writing v1 state, and trace_contributions/ is the shared trace-commons state root the Reborn runtime itself consumes. If the subsystem migrates to the Reborn home it must migrate wholesale (resolver + all commands + worker) in one change, not one command at a time; happy to file that as a tracked follow-up if the migration is wanted.
The doc comment still pointed users at bare 'traces opt-out' for self-exclusion, which is now the instance-wide off switch; name the --user-scope form and warn about the bare form, matching the runtime output. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
CodeRabbit's outside-diff finding (stale |
1 participant
Summary
Closes the two gaps that made instance-wide Trace Commons enrollment (PR #5280's "Path B") unusable in practice:
onboarding::onboard_instance_with_sinkexisted but nothing called it (theAdminScopewrapper was deliberately removed from the v1 monolith during Trace Commons: instance-wide enrollment, per-user profiles, and trace inspection #5280 review, deferring a Reborn-side surface).Admin half —
ironclaw-reborn traces enroll-instanceThin CLI wrapper over the new
onboarding::onboard_instance_at_base(device-key invite onboarding targeting the scope-Nonelocation). Host-shell possession is the admin gate — the same trust boundarytraces opt-inalready uses to write the global policy. Every user without a personal enrollment inherits the enrollment with a salted per-user pseudonymous subject;traces opt-outstill wins.User half — Open Trace Commons account from the WebUI
RebornServicesApi::trace_account_login_link→POST /api/webchat/v2/traces/account-login-link(caller-scoped, NoBody, per-caller 10/min rate limit since each call mints a credential).mint_account_login_linkdirect variant inironclaw_reborn_tracesbuilt on a new productionDirectPinnedContributionSink(pinned DNS + private-IP rejection + streaming-bounded body — same hardening as the other direct clients).noopener,noreferrer, then navigates it to the minted URL; placeholder closed on unenrolled/error. i18n for all 11 languages.Drive-by fix
trace-commons-tab.test.mjshad silently stopped running when the frontend moved to vitest (include pattern is*.{test,spec}.{ts,tsx}). Converted totrace-commons-tab.test.ts; the suite count goes 582 → 590 and thewindow.openfake captures every argument the production caller passes.Testing
onboard_instance_at_base_targets_the_instance_dir(policy + promoted device key attrace_contributions/, nousers/<hash>dir)--invite)openAccountLoginLinkcovered for opened/unavailable/error/blocked; full-argswindow.opencapture🤖 Generated with Claude Code