Harden tier2 scoped writeback harness#233
Conversation
|
Warning Review limit reached
More reviews will be available in 35 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds HTTP response status logging to mount client operations and extends the Tier-2 load harness to support Relayauth API key-driven token minting with dedicated data-plane token scoping. It also records completed trajectory metadata for prior review work and updates the trajectory index. ChangesHTTP Status Logging Feature
Tier-2 Load Harness Credential and Token Minting Updates
Trajectory Completion Metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces HTTP status logging for the mount client to improve observability, adding a --log-http-status flag and corresponding logging logic. It also updates the load test harness to support dynamically minting workspace and data-plane tokens. A concurrency issue was identified in HTTPClient where httpStatusLogger is accessed concurrently without synchronization, which could lead to a data race; protecting it with a mutex is recommended.
| func (c *HTTPClient) SetHTTPStatusLogger(logger Logger) { | ||
| c.httpStatusLogger = logger | ||
| } | ||
|
|
||
| func (c *HTTPClient) logHTTPStatus(method, requestPath string, statusCode int, retryAfter string, attempt int) { | ||
| if c.httpStatusLogger == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
The httpStatusLogger field of HTTPClient is read concurrently in logHTTPStatus (which is called by concurrent goroutines during bootstrap/sync) and can be written concurrently via SetHTTPStatusLogger. Since Go interface values are not safe for concurrent read/write without synchronization, this introduces a data race that can lead to panics or undefined behavior. Protecting access to httpStatusLogger using the existing tokenMu RWMutex resolves this issue safely.
func (c *HTTPClient) SetHTTPStatusLogger(logger Logger) {
c.tokenMu.Lock()
c.httpStatusLogger = logger
c.tokenMu.Unlock()
}
func (c *HTTPClient) logHTTPStatus(method, requestPath string, statusCode int, retryAfter string, attempt int) {
c.tokenMu.RLock()
logger := c.httpStatusLogger
c.tokenMu.RUnlock()
if logger == nil {
return
}
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
|
Reviewed and fixed the PR locally. Changed:
Verified:
|
|
✅ pr-reviewer applied fixes — committed and pushed Reviewed and fixed the PR locally. Changed:
Verified:
|
![agent-relay-bot[bot]](https://avatars.githubusercontent.com/in/3624277?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ pr-reviewer applied fixes — committed and pushed 3df8453 to this PR. The notes below describe what changed.
Reviewed and fixed the PR locally.
Changed:
- Wired
--log-http-statusthrough FUSE mounts incmd/relayfile-mount/fuse_mount.go. - Made
HTTPClientstatus logger access race-safe ininternal/mountsync/syncer.go. - Updated
test/load/README.mdso the tier2 docs match the newRELAYFILE_TIER2_RELAYAUTH_API_KEYminting path.
Verified:
npm run test:load:tier2-scoped-writeback:self-testpasses.- Go tests could not run because
gois not installed in this environment.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
khaliqgant
force-pushed
the
codex/tier2-workspace-token
branch
from
3df8453 to
033b792
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/load/tier2-cfdo-scoped-writeback.mjs`:
- Around line 377-405: The minted token must be validated to ensure its decoded
scopes are confined to runRoot: in mintRunDataPlaneToken, after calling
scopesFromToken(accessToken) (and using tokenPath(runRoot) as the requested
path), inspect the decoded scopes and/or response.body?.paths and reject the
token if any granted path does not start with the runRoot prefix or grants
broader access than intended; throw an error if scopes are missing, empty, or
include any path outside runRoot so only tokens scoped to the runRoot are
accepted.
🪄 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: d7d27fd4-acb4-43a9-821c-7985505846c6
📒 Files selected for processing (29)
.trajectories/completed/2026-04/traj_7x9nltybo08h.compaction.json.trajectories/completed/2026-04/traj_82lywlk9dcnc.compaction.json.trajectories/completed/2026-04/traj_cdist8i8vdmd.compaction.json.trajectories/completed/2026-04/traj_dmoc4slub7ox.compaction.json.trajectories/completed/2026-04/traj_em3hvzpg1xmx.compaction.json.trajectories/completed/2026-04/traj_i1f02867dkxn.compaction.json.trajectories/completed/2026-04/traj_iuzm83ogm43k.compaction.json.trajectories/completed/2026-04/traj_nixaonkglri1.compaction.json.trajectories/completed/2026-04/traj_qi3qmy5oveab.compaction.json.trajectories/completed/2026-04/traj_wez7rl7pkfpn.compaction.json.trajectories/completed/2026-05/traj_6fjv0fnvrc5e.compaction.json.trajectories/completed/2026-05/traj_6lyjg41p6a28.compaction.json.trajectories/completed/2026-05/traj_9khc36ax639i.compaction.json.trajectories/completed/2026-05/traj_a6rfc30zag40.compaction.json.trajectories/completed/2026-05/traj_ailh4waboewf.compaction.json.trajectories/completed/2026-05/traj_d3drzvodqpn7.compaction.json.trajectories/completed/2026-05/traj_hyqnsfininh5.compaction.json.trajectories/completed/2026-05/traj_v1un6n66y38i.compaction.json.trajectories/completed/2026-05/traj_xf18gkmtr3ib.compaction.json.trajectories/completed/2026-05/traj_z2klijcrwqed.compaction.json.trajectories/completed/2026-06/traj_9n5xjmq2qpxz/summary.md.trajectories/completed/2026-06/traj_9n5xjmq2qpxz/trajectory.json.trajectories/index.jsoncmd/relayfile-mount/fuse_mount.gocmd/relayfile-mount/main.gointernal/mountsync/http_client_test.gointernal/mountsync/syncer.gotest/load/README.mdtest/load/tier2-cfdo-scoped-writeback.mjs
💤 Files with no reviewable changes (1)
- .trajectories/index.json
| async function mintRunDataPlaneToken(config) { | ||
| const runRoot = `/team-tier2/${config.runId}`; | ||
| const response = await requestJSON(config, { | ||
| api: "relayauth", | ||
| method: "POST", | ||
| path: "/v1/tokens/path", | ||
| token: config.workspaceToken, | ||
| body: { | ||
| workspaceId: config.workspaceId, | ||
| paths: [tokenPath(runRoot)], | ||
| ttlSeconds: config.tokenTtlSeconds, | ||
| agentName: "tier2-data-plane", | ||
| }, | ||
| }); | ||
| if (!response.ok) { | ||
| throw new Error(`data-plane token mint failed: ${response.status} ${JSON.stringify(response.body ?? response.text)}`); | ||
| } | ||
| const accessToken = response.body?.accessToken; | ||
| if (typeof accessToken !== "string" || accessToken.trim() === "") { | ||
| throw new Error("data-plane token mint did not return accessToken"); | ||
| } | ||
| return { | ||
| token: accessToken, | ||
| scopes: scopesFromToken(accessToken), | ||
| paths: response.body?.paths, | ||
| status: response.status, | ||
| durationMs: response.durationMs, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Validate the run data-plane token scopes before accepting it.
This helper decodes the returned scopes but never enforces that the minted token is actually confined to runRoot. If /v1/tokens/path ever returns a broader write token, the harness can still pass while using that token for seeding, reads, and admission probes, which weakens the scoped-correctness claim.
Suggested fix
+function validateRunDataPlaneScopes(scopes, runRoot) {
+ if (!Array.isArray(scopes) || scopes.length === 0) {
+ throw new Error(`data-plane token for ${runRoot} has no scopes`);
+ }
+ const requiredWriteScope = pathScope(runRoot);
+ let writeScopes = 0;
+ for (const scope of scopes) {
+ if (isBroadOrAdminScope(scope)) {
+ throw new Error(`data-plane token for ${runRoot} has broad/admin scope ${scope}`);
+ }
+ const value = String(scope ?? "").trim();
+ if (value.startsWith("relayfile:fs:write:")) {
+ writeScopes += 1;
+ if (value !== requiredWriteScope) {
+ throw new Error(`data-plane token for ${runRoot} has wrong-root write scope ${scope}`);
+ }
+ }
+ }
+ if (writeScopes === 0) {
+ throw new Error(`data-plane token for ${runRoot} is missing exact write scope ${requiredWriteScope}`);
+ }
+}
+
async function mintRunDataPlaneToken(config) {
const runRoot = `/team-tier2/${config.runId}`;
const response = await requestJSON(config, {
@@
if (typeof accessToken !== "string" || accessToken.trim() === "") {
throw new Error("data-plane token mint did not return accessToken");
}
+ const scopes = scopesFromToken(accessToken);
+ validateRunDataPlaneScopes(scopes, runRoot);
return {
token: accessToken,
- scopes: scopesFromToken(accessToken),
+ scopes,
paths: response.body?.paths,
status: response.status,
durationMs: response.durationMs,
};
}🤖 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 `@test/load/tier2-cfdo-scoped-writeback.mjs` around lines 377 - 405, The minted
token must be validated to ensure its decoded scopes are confined to runRoot: in
mintRunDataPlaneToken, after calling scopesFromToken(accessToken) (and using
tokenPath(runRoot) as the requested path), inspect the decoded scopes and/or
response.body?.paths and reject the token if any granted path does not start
with the runRoot prefix or grants broader access than intended; throw an error
if scopes are missing, empty, or include any path outside runRoot so only tokens
scoped to the runRoot are accepted.
|
Reviewed PR #233 and traced the changed mount, HTTP client, and tier2 harness paths. The bot review’s actionable logger race finding is already fixed with synchronized logger access. I restored
Go package tests could not run because |
There was a problem hiding this comment.
1) — fixes were not applied to the PR. The notes below are advisory and were not pushed.
Reviewed PR #233 and traced the changed mount, HTTP client, and tier2 harness paths. The bot review’s actionable logger race finding is already fixed with synchronized logger access.
I restored .trajectories/index.json; the PR was deleting it, which would break the workspace trajectory metadata contract. I also verified the tier2 harness self-test passes:
npm run test:load:tier2-scoped-writeback:self-test
Go package tests could not run because go is not installed in this environment.
|
Reviewed PR #233 and made one fix in Checks run:
I could not run Go tests locally because |
There was a problem hiding this comment.
Reviewed PR #233 and made one fix in test/load/tier2-cfdo-scoped-writeback.mjs: the harness now validates the minted run data-plane token’s decoded write scope before using it for seed/verify/probe calls. This keeps the new “run-scoped data-plane token” contract enforceable instead of only recording scopes in evidence.
Checks run:
node test/load/tier2-cfdo-scoped-writeback.mjs --self-testnode --check test/load/tier2-cfdo-scoped-writeback.mjsscripts/check-contract-surface.sh
I could not run Go tests locally because go is not installed in this environment.
|
Reviewed and fixed PR #233 locally. Changes made:
Checks run:
Go tests could not be run because |
There was a problem hiding this comment.
Reviewed and fixed PR #233 locally.
Changes made:
test/load/tier2-cfdo-scoped-writeback.mjs- Prefer an explicit
RELAYFILE_TIER2_WORKSPACE_TOKENwhen both token and API key are set. - Only mint a workspace token from
RELAYFILE_TIER2_RELAYAUTH_API_KEYwhen no workspace token is provided. - Validate the minted run data-plane token’s decoded scopes before using it.
- Fix root token path normalization from
//*to/*. - Added self-test coverage for these cases.
- Prefer an explicit
test/load/README.md- Documented credential precedence when both workspace token and API key are present.
packages/sdk/typescript/src/workspace-seeder-tar.test.ts- Removed stale
@relayfile/sdkpackage-entry mock that broke clean-install SDK tests beforedistexists.
- Removed stale
Checks run:
npm run test:load:tier2-scoped-writeback:self-testnode --check test/load/tier2-cfdo-scoped-writeback.mjsscripts/check-contract-surface.shnpm run test --workspace=packages/corenpm run test --workspace=packages/sdk/typescriptnpm run test --workspace=packages/local-mountnpm run test --workspace=@relayfile/file-observernpm run typecheck --workspace=packages/sdk/typescriptnpm run typecheck --workspace=packages/local-mountnpm run build --workspace=packages/corenpm run build --workspace=packages/sdk/typescript
Go tests could not be run because go is not installed in this environment.
|
Reviewed PR #233 and fixed two real breakages found during local verification:
Local checks passed:
|
There was a problem hiding this comment.
Reviewed PR #233 and fixed two real breakages found during local verification:
cmd/relayfile-cli/main.go: flushed OAuth callback error responses before signalingrunCloudLogin, preventing EOF races in the state-mismatch test.packages/local-mount/src/auto-sync.tsandmount-reflink.test.ts: tracked file sizes alongside mtimes so rapid same-tick edits are detected, and made the reflink copy test deterministic.
Local checks passed:
PATH=/tmp/codex-go/go/bin:$PATH npm test./scripts/check-contract-surface.sh- Go builds for
relayfile,relayfile-mount, andrelayfile-cli npm run test:load:tier2-scoped-writeback:self-test
|
Reviewed and fixed PR #233 locally. Changes made:
Local checks passed:
|
There was a problem hiding this comment.
Reviewed and fixed PR #233 locally.
Changes made:
- Tier-2 harness now prefers explicit workspace tokens, validates run data-plane token scopes, and fixes root token path normalization.
- README documents credential precedence.
- Fixed unpushed bot-review CI issues in SDK tar seeder test, local-mount same-tick autosync detection, and CLI OAuth callback flushing.
Local checks passed:
go test ./...scripts/check-contract-surface.sh- Tier-2 harness self-test and syntax check
- SDK/core/local-mount/file-observer tests
- SDK/local-mount typechecks
- core and SDK builds
|
Reviewed PR #233 and made one fix in Verified:
I could not run Go tests because |
There was a problem hiding this comment.
Reviewed PR #233 and made one fix in test/load/tier2-cfdo-scoped-writeback.mjs: tokenPath("/") now returns "/*" instead of "//*", with self-test coverage added.
Verified:
npm run test:load:tier2-scoped-writeback:self-testpasses- credential-gated harness skip exits
77and emits skipped evidence as expected
I could not run Go tests because go/gofmt are not installed in this container.
1 participant
Summary
Production evidence already captured
Verification
Notes