696 unified session#697
Merged
Merged
Conversation
Closed
|
Important Review skippedToo many files! This PR contains 172 files, which is 72 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (172)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds observe-session connectivity across core, collector, bundling, and CLI packages; changes ingest posts to versioned envelopes; tightens contract validation; adds cookie-domain session storage; updates preview output and documentation; and refreshes release metadata and workflow action pins. ChangesObserve connectivity
Contract validation
Session cookie storage
Release and workflow maintenance
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/website.yml:
- Line 35: Add persist-credentials: false to each actions/checkout step in
.github/workflows/website.yml (35-35), .github/workflows/storybook.yml (27-27),
and .github/workflows/tagging.yml (23-23), preserving the existing checkout
configuration.
In `@packages/cli/src/commands/bundle/bundler.ts`:
- Around line 1978-1979: Update the connect-config conversion near the
trimmedUrl/trimmedBinding return to preserve the configured level and sample
fields when constructing the Flow.Observe result, including level: 'off'. Update
the corresponding observe codegen test coverage to assert these controls survive
conversion and prevent telemetry defaults from overriding explicit settings.
In `@packages/cli/src/commands/observe/index.ts`:
- Around line 91-95: Update the timeout parsing and polling loop around
getObserveSession to require a fully valid numeric timeout instead of accepting
trailing text or truncating fractional values. In the loop, calculate the
remaining time before sleeping and cap the polling delay to that remainder so no
sleep crosses the deadline; return the session once the deadline is reached.
In `@packages/collector/src/flow.ts`:
- Around line 158-231: Apply observe.level consistently in the observe
initialization flow: in the web arm, return immediately for level 'off' before
reading credentials, writing storage, or creating an observer; in the server
arm, ensure level 'trace' installs observeLevel so destination-call capture is
enabled. Update the wiring tests to cover web level 'off' and server level
'trace', preserving existing behavior for other levels.
In `@packages/core/src/batchedPoster.ts`:
- Around line 205-209: Filter caller headers case-insensitively in the header
construction around batchedPoster request options, removing names whose
name.toLowerCase() is authorization or content-type before adding the fixed
values. In packages/core/src/batchedPoster.ts lines 205-209, preserve only the
fixed reserved headers; in packages/core/src/__tests__/batchedPoster.test.ts
lines 231-259, add lowercase authorization and content-type collision cases
verifying the fixed values are the only reserved headers.
In `@packages/core/src/observeConnect.ts`:
- Around line 87-88: Update the WALKEROS_OBSERVE_LEVEL handling in
observeConnect so an explicitly provided value that fails isLevel is rejected
rather than ignored; preserve the existing assignment for valid levels and
ensure invalid configuration cannot fall back to active standard observation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bad9029f-994b-46de-94ff-196d31e596ff
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (42)
.changeset/cli-contract-shape-validation.md.changeset/collector-push-dropped-flag.md.changeset/observe-connect-foundation.md.github/workflows/release.yml.github/workflows/storybook.yml.github/workflows/tagging.yml.github/workflows/test.yml.github/workflows/website.ymlpackage.jsonpackages/cli/src/__tests__/integration/trace-loop.test.tspackages/cli/src/__tests__/unit/bundle/preview-codegen.test.tspackages/cli/src/__tests__/unit/bundle/wrap.test.tspackages/cli/src/__tests__/unit/validate/contract.test.tspackages/cli/src/cli.tspackages/cli/src/commands/bundle/__tests__/observe-codegen.test.tspackages/cli/src/commands/bundle/bundler.tspackages/cli/src/commands/bundle/wrap.tspackages/cli/src/commands/observe/__tests__/observe.test.tspackages/cli/src/commands/observe/index.tspackages/cli/src/commands/validate/__tests__/contract.test.tspackages/collector/src/__tests__/observe-wiring.test.tspackages/collector/src/flow.tspackages/core/src/__tests__/batchedPoster-seq-split.test.tspackages/core/src/__tests__/batchedPoster.test.tspackages/core/src/__tests__/observeConnect.test.tspackages/core/src/__tests__/preview-activator.test.tspackages/core/src/__tests__/preview.test.tspackages/core/src/batchedPoster.tspackages/core/src/index.tspackages/core/src/observeConnect.tspackages/core/src/preview.tspackages/core/src/schemas/collector.tspackages/core/src/schemas/flow.tspackages/core/src/types/collector.tspackages/core/src/types/flow.tspackages/core/src/types/telemetry.tsskills/walkeros-using-cli/SKILL.mdwebsite/docs/apps/cli.mdxwebsite/docs/getting-started/flow/contract.mdxwebsite/docs/getting-started/modes/bundled.mdxwebsite/docs/getting-started/modes/integrated.mdxwebsite/docs/getting-started/observe.mdx
💤 Files with no reviewable changes (1)
- packages/cli/src/commands/validate/tests/contract.test.ts
| TURBO_TEAM: ${{ secrets.TURBO_TEAM }} | ||
| steps: | ||
| - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 | ||
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable persisted checkout credentials in deployment workflows.
These jobs do not need Git credentials after checkout, so retaining the token exposes it to subsequent build/deployment commands. Add persist-credentials: false at each checkout:
.github/workflows/website.yml#L35-L35: harden the website and PR-preview deployment job..github/workflows/storybook.yml#L27-L27: harden the Storybook deployment job..github/workflows/tagging.yml#L23-L23: harden the tagging deployment job.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 35-35: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
📍 Affects 3 files
.github/workflows/website.yml#L35-L35(this comment).github/workflows/storybook.yml#L27-L27.github/workflows/tagging.yml#L23-L23
🤖 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/website.yml at line 35, Add persist-credentials: false to
each actions/checkout step in .github/workflows/website.yml (35-35),
.github/workflows/storybook.yml (27-27), and .github/workflows/tagging.yml
(23-23), preserving the existing checkout configuration.
Source: Linters/SAST tools
Comment on lines
+158
to
+231
| const observe = initConfig.observe; | ||
| if (!observe) return; | ||
|
|
||
| const flowId = instance.name ?? 'default'; | ||
|
|
||
| if ('binding' in observe) { | ||
| // Web arm: the credential arrives out-of-band via the elbObserve URL | ||
| // param (or the storage slot the preview activator ferries it into). | ||
| // Truly absent means zero work: no parse, no storage write, no observer. | ||
| const param = readObserveParam(); | ||
| const raw = param ?? readObserveSlot(); | ||
| if (!raw) return; | ||
|
|
||
| // A present-but-broken credential is warned, never silently dropped; | ||
| // only the truly-absent case above is silent (zero-work contract). | ||
| const credential = parseObserveCredential(raw); | ||
| if (!credential) { | ||
| if (param === null) { | ||
| // Slot-sourced garbage: stored state clears on its own failure, so | ||
| // self-heal instead of warning on every pageview. | ||
| instance.logger.warn('observe: malformed stored credential cleared'); | ||
| clearObserveSlotIf(raw); | ||
| } else { | ||
| // URL-sourced garbage never touches stored state (anti-griefing). | ||
| instance.logger.warn('observe: malformed credential ignored'); | ||
| } | ||
| return; | ||
| } | ||
| if (credential.pb !== observe.binding) { | ||
| instance.logger.warn( | ||
| 'observe: credential binding mismatch, credential ignored', | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| writeObserveSlot(raw); | ||
| // The credential is persisted; a URL-borne copy is now redundant and, | ||
| // being a bearer credential, must not linger in the address bar. | ||
| if (param !== null) stripObserveParam(); | ||
|
|
||
| let observer: ObserverFn | undefined; | ||
| const post = createBatchedPoster({ | ||
| url: `${observe.url}/ingest/${credential.ses}`, | ||
| token: credential.secret, | ||
| headers: { 'X-Walkeros-Binding': observe.binding }, | ||
| onStatus: (status) => { | ||
| if (status !== 401) return; | ||
| // The observer rejected this credential (session ended or token | ||
| // revoked): self-heal the slot -- CAS, only while it still holds | ||
| // exactly this credential -- and detach, so posting stops and the | ||
| // next pageview is back on the silent zero-work path. | ||
| clearObserveSlotIf(raw); | ||
| if (observer) instance.observers.delete(observer); | ||
| }, | ||
| }); | ||
| // Baked scoping: a wrap (e.g. a preview artifact) may carry a public | ||
| // flowId/level/sample on the connect config. flowId overrides the local | ||
| // config name on every posted record so they land under the platform's | ||
| // flow id; a baked level also drives the collector-wide capture supplier | ||
| // so destination call capture runs at trace. | ||
| observer = createTelemetryObserver( | ||
| withFlowId(withRelease(post, instance.release), observe.flowId), | ||
| { | ||
| flowId: observe.flowId ?? flowId, | ||
| ...(observe.level !== undefined ? { level: observe.level } : {}), | ||
| ...(observe.sample !== undefined ? { sample: observe.sample } : {}), | ||
| }, | ||
| ); | ||
| instance.observers.add(observer); | ||
| const bakedLevel = observe.level; | ||
| if (bakedLevel !== undefined) { | ||
| instance.observeLevel = () => bakedLevel; | ||
| } | ||
| return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply observe.level consistently across both runtime arms.
The web arm does not honor off, while the server arm never installs observeLevel for trace; this leaves web observation doing unnecessary credential/observer work and server traces missing destination-call capture.
Proposed fix
const observe = initConfig.observe;
- if (!observe) return;
+ if (!observe || observe.level === 'off') return;
const flowId = instance.name ?? 'default';
if ('binding' in observe) {
// Existing web setup...
}
// Server arm: the config trio is the credential; attach directly.
- // 'off' opts out entirely (no observer, zero emit-path work).
- if (observe.level === 'off') return;
+ const serverLevel = observe.level;
+ if (serverLevel !== undefined) {
+ instance.observeLevel = () => serverLevel;
+ }
instance.observers.add(Please also cover web level: 'off' and server level: 'trace' in the wiring tests.
Also applies to: 234-248
🤖 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 `@packages/collector/src/flow.ts` around lines 158 - 231, Apply observe.level
consistently in the observe initialization flow: in the web arm, return
immediately for level 'off' before reading credentials, writing storage, or
creating an observer; in the server arm, ensure level 'trace' installs
observeLevel so destination-call capture is enabled. Update the wiring tests to
cover web level 'off' and server level 'trace', preserving existing behavior for
other levels.
Contributor
Preview deployed |
alexanderkirtzel
temporarily deployed
to
npm-publish
July 18, 2026 11:09 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
📦 Pre-release published (next) Packages Install: 🐳 Docker images published
Docker: |
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 (4)
website/docs/getting-started/observe.mdx (2)
50-55: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse a non-credential placeholder in the activation URL.
The page states that credentials never live in committed artifacts, but this example contains a credential-shaped literal (
obsw_pb_x.ses_9.sec_1). Use?elbObserve=<session-credential>instead to avoid ambiguity and accidental reuse.🤖 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 `@website/docs/getting-started/observe.mdx` around lines 50 - 55, Update the activation URL example in the CodeSnippet to use the non-credential placeholder `?elbObserve=<session-credential>` instead of the credential-shaped literal, while preserving the surrounding URL and documentation.
119-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the contradictory server example comment.
The comment says
observebecomes configured when all three variables are set, “then zero observation work” occurs. This reverses the documented behavior below; observation should be enabled when the full trio is present, and disabled when it is absent.Proposed wording
- // undefined unless all three variables are set: then zero observation work + // undefined unless all three variables are set; otherwise, no observation work🤖 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 `@website/docs/getting-started/observe.mdx` around lines 119 - 123, Update the inline comment beside observeFromEnv in the startFlow example to state that observation is enabled when all three environment variables are set and disabled when they are absent, matching the documented behavior below.packages/cli/src/commands/observe/index.ts (1)
119-136: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate programmatic polling numbers before entering the loop.
The exported API accepts
NaN,Infinity, and negative values. These can produce a tight request loop or a deadline that never expires.Proposed fix
export async function waitForObserveSessionSettled(options: { projectId?: string; flowId: string; sessionId: string; timeoutMs: number; pollIntervalMs: number; }): Promise<ObserveSessionResponse> { + if (!Number.isFinite(options.timeoutMs) || options.timeoutMs < 0) { + throw new Error('timeoutMs must be a finite, non-negative number'); + } + if ( + !Number.isFinite(options.pollIntervalMs) || + options.pollIntervalMs < 0 + ) { + throw new Error('pollIntervalMs must be a finite, non-negative number'); + } + const deadline = Date.now() + options.timeoutMs;🤖 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 `@packages/cli/src/commands/observe/index.ts` around lines 119 - 136, Validate options.timeoutMs and options.pollIntervalMs at the start of waitForObserveSessionSettled, rejecting NaN, non-finite, or negative values before the polling loop begins; preserve normal polling behavior for valid non-negative finite numbers.packages/cli/openapi/spec.json (1)
2495-2526: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRemove or deprecate
forcefromCreateObserveSessionRequest
packages/cli/openapi/spec.json:2495-2526still exposesforcealongsidereplace, but the CLI now only sendsreplace. Ifforceis kept for compatibility, mark it deprecated and document the overlap; otherwise drop it from the schema and regenerated client types.🤖 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 `@packages/cli/openapi/spec.json` around lines 2495 - 2526, Update the CreateObserveSessionRequest schema to remove the obsolete force property, since callers now use replace; regenerate any client types derived from this OpenAPI definition so force is no longer exposed. If compatibility requires retaining force, instead mark it deprecated and document its overlap with replace.
🤖 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 `@packages/cli/src/commands/bundle/bundler.ts`:
- Around line 1991-1993: Update the sample validation in the bundling logic
around the sample object construction to accept only finite numbers in the
inclusive range [0, 1], omitting out-of-range values while preserving valid
samples. Add boundary and out-of-range coverage in observe-codegen.test.ts.
In `@packages/cli/src/commands/observe/__tests__/observe.test.ts`:
- Around line 566-582: Reset the console output spies inside each iteration of
the invalid-timeout loop before calling observeStartCommand, so
consoleErrorText() only reflects the current case and each timeout independently
verifies its own “Invalid --timeout” error.
---
Outside diff comments:
In `@packages/cli/openapi/spec.json`:
- Around line 2495-2526: Update the CreateObserveSessionRequest schema to remove
the obsolete force property, since callers now use replace; regenerate any
client types derived from this OpenAPI definition so force is no longer exposed.
If compatibility requires retaining force, instead mark it deprecated and
document its overlap with replace.
In `@packages/cli/src/commands/observe/index.ts`:
- Around line 119-136: Validate options.timeoutMs and options.pollIntervalMs at
the start of waitForObserveSessionSettled, rejecting NaN, non-finite, or
negative values before the polling loop begins; preserve normal polling behavior
for valid non-negative finite numbers.
In `@website/docs/getting-started/observe.mdx`:
- Around line 50-55: Update the activation URL example in the CodeSnippet to use
the non-credential placeholder `?elbObserve=<session-credential>` instead of the
credential-shaped literal, while preserving the surrounding URL and
documentation.
- Around line 119-123: Update the inline comment beside observeFromEnv in the
startFlow example to state that observation is enabled when all three
environment variables are set and disabled when they are absent, matching the
documented behavior below.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1dad4f4e-66ae-4175-a138-860d834f3cbf
📒 Files selected for processing (27)
.changeset/retire-telemetry-bake.md.github/workflows/storybook.yml.github/workflows/tagging.yml.github/workflows/website.ymlpackages/cli/openapi/spec.jsonpackages/cli/src/__tests__/unit/bundle/bundler-codegen.test.tspackages/cli/src/__tests__/unit/bundle/preview-codegen.test.tspackages/cli/src/__tests__/unit/bundle/wrap.test.tspackages/cli/src/cli.tspackages/cli/src/commands/bundle/__tests__/bundler.test.tspackages/cli/src/commands/bundle/__tests__/observe-codegen.test.tspackages/cli/src/commands/bundle/bundler.tspackages/cli/src/commands/bundle/wrap.tspackages/cli/src/commands/observe/__tests__/observe.test.tspackages/cli/src/commands/observe/index.tspackages/cli/src/commands/previews/__tests__/output.test.tspackages/cli/src/commands/previews/__tests__/previews.test.tspackages/cli/src/commands/previews/output.tspackages/cli/src/types/api.gen.d.tspackages/collector/src/__tests__/observe-wiring.test.tspackages/collector/src/flow.tspackages/core/src/__tests__/batchedPoster.test.tspackages/core/src/__tests__/observeConnect.test.tspackages/core/src/batchedPoster.tspackages/core/src/observeConnect.tsskills/walkeros-using-cli/commands-reference.mdwebsite/docs/getting-started/observe.mdx
💤 Files with no reviewable changes (1)
- packages/cli/src/commands/bundle/tests/bundler.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- .github/workflows/storybook.yml
- .github/workflows/tagging.yml
- packages/core/src/tests/observeConnect.test.ts
- packages/core/src/observeConnect.ts
- packages/core/src/tests/batchedPoster.test.ts
- packages/cli/src/cli.ts
- .github/workflows/website.yml
- packages/collector/src/tests/observe-wiring.test.ts
- packages/collector/src/flow.ts
- packages/core/src/batchedPoster.ts
Contributor
|
🚀 Stable release published Packages Install: 🐳 Docker images published
Docker: |
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 by CodeRabbit
walkeros observe startfor live observation sessions (web/server connect, polling/settle control, JSON output).tagging/schematypes, and warns on unknown keys.