feat(digest): PR-39 workspace-primitives digest runtime (relayfile slice)#162
Closed
khaliqgant wants to merge 8 commits into
Closed
feat(digest): PR-39 workspace-primitives digest runtime (relayfile slice)#162khaliqgant wants to merge 8 commits into
khaliqgant wants to merge 8 commits into
Conversation
Implements the digest/activity-summary gaps from docs/workspace-primitives-pr39-gap-spec.md: - internal/digest: date-stamped (YYYY-MM-DD), this-week, last-week, today, yesterday window builders with workspace-timezone support and golden fixtures. - cmd/relayfile-cli: `relayfile digest rebuild --window ...` real implementation (was a stub) plus tests. - internal/mountfuse: virtual LAYOUT.md / digest exposure in the FUSE layer. - internal/mountsync: digest scheduler, digest-source event filtering (no recursion on /digests/* writes), digest writer + watcher tests. - packages/sdk/python: expanded digest path taxonomy. - .claude/rules/relayfile-integration-digests.md: digest runtime contract rule. - docs/workspace-primitives-pr39-gap-spec.md: the gap spec this implements, with resolved decisions. Generated by a Ricky master-executor run (12 child slices); only the product source/tests/spec are committed here. Runtime digest output (digests/*.md), ricky scratch (.workflow-artifacts/, workflows/generated/, internal/digest/cmd/ demos, mount-verify/), and trail bookkeeping are intentionally excluded. Verified: go build ./... clean; go test ./internal/digest ./internal/mountfuse ./internal/mountsync ./cmd/relayfile-cli all pass; pytest packages/sdk/python 9 passed; scripts/check-contract-surface.sh passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
trail compact --discard-sources for the workspace-primitives PR-39 master run: the two completed source trajectories are removed and index.json updated to the compacted record. No source trajectory is still referenced by the index. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds complete digest runtime: timezone-aware windows (today/yesterday/this-week/last-week/date-stamped), idempotent/atomic writers and markers, anti-recursion path filtering, rolling coalescer and close scheduler, CLI rebuilder wiring, provider-layout and SDK updates, extensive tests and fixtures, and documentation/spec additions. ChangesDigest & Scheduling
Mountsync / Syncer Integration
CLI: rebuilder and workspace timezone
Provider layout and FUSE
Python SDK & docs
Tests & Fixtures
Sequence Diagram(s) sequenceDiagram
participant CLI as relayfile CLI
participant Rebuilder as localDigestRebuilder
participant Digest as internal/digest
participant FS as MountedFS (watcher)
participant Syncer as mountsync Syncer
CLI->>Rebuilder: Rebuild(window, tz, now)
Rebuilder->>Digest: Render/Write for cover (today/yesterday/this-week/last-week/date-stamped)
Digest->>FS: atomic rename/write digests/* (emit file events)
FS->>Syncer: watcher emits file events
Syncer->>Digest: runClosing/runRolling jobs (Tick/ObserveChange)
Estimated code review effort: Possibly related PRs:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
# Conflicts: # internal/mountsync/syncer.go
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. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+60
to
+61
| c.pending = true | ||
| c.dueAt = now.Add(interval) |
Contributor
There was a problem hiding this comment.
🔴 RollingDigestCoalescer starves under sustained changes — dueAt resets on every ObserveChange
Every call to ObserveChange unconditionally resets c.dueAt = now.Add(interval) (internal/mountsync/scheduler.go:61). If provider changes arrive more frequently than the 30-second interval, the deadline is perpetually pushed forward and Due() never returns true. This means runRollingDigestJobsLocked at internal/mountsync/syncer.go:1482 always sees !s.rollingCoalescer.Due() and skips the rolling digest write, so today.md and this-week.md are never regenerated under sustained load.
The spec (docs/workspace-primitives-pr39-gap-spec.md:93-94) says "Rolling windows rebuild on every relevant change, coalesced to at most once per 30 seconds", meaning at most 30 seconds of delay — not indefinite starvation. The fix should track the timestamp of the first un-flushed change and fire when that exceeds the interval, rather than resetting the deadline on every new change.
Starvation scenario under continuous 10-second changes
- t=0s: change → dueAt=30s
- t=10s: change → dueAt=40s
- t=20s: change → dueAt=50s
- t=30s: SyncOnce runs, Due() checks 30 >= 50 → false, skip
- t=30s: change → dueAt=60s
- … repeats indefinitely, digest never written
Prompt for agents
In RollingDigestCoalescer.ObserveChange (internal/mountsync/scheduler.go), the line `c.dueAt = now.Add(interval)` resets the deadline on every change, causing indefinite starvation under sustained load. The fix is to only set dueAt when transitioning from non-pending to pending (i.e. the first change after a flush). When already pending, a new change should NOT push dueAt forward.
Concretely: add a guard like `if !c.pending { c.dueAt = now.Add(interval) }` before `c.pending = true`. This way the deadline is anchored to the first un-flushed change, and Due() will fire interval seconds after that first change regardless of how many subsequent changes arrive. The existing test TestCoalescer_30s only tests a single change; add a test with rapid repeated ObserveChange calls to prove the coalescer fires after the interval even under sustained load.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
.trajectories/index.json (1)
239-1357: 💤 Low valueConsider investigating the batch creation of abandoned trajectories.
Over 100 abandoned trajectories were created within a 70-second window on 2026-05-13 (19:30:20 to 19:31:22), each with a lifespan of 1-2 seconds. All follow the pattern "ricky-child-update-*-workflow". This suggests an automated process may have spawned many short-lived child trajectories that were quickly abandoned.
While this doesn't break functionality, it adds significant noise to the trajectory index and may indicate an issue with the trajectory creation logic or a batch operation that encountered repeated failures.
🤖 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 @.trajectories/index.json around lines 239 - 1357, Many short-lived "ricky-child-update-*-workflow" trajectories were created and abandoned in a tight batch, so locate the trajectory creation site(s) (look for functions/methods named createTrajectory, spawnChildTrajectory, TrajectoryManager.create or WorkflowRunner.spawnChild) and add protections: rate-limit/debounce rapid successive calls, check for existing active trajectories with the same title/tag before creating a new one (dedupe), and implement retry/backoff plus clearer logging/error propagation so failures don't trigger repeated immediate child creations and automatic abandonment.docs/workspace-primitives-pr39-gap-spec.md (1)
315-318: 💤 Low valueConsider clarifying the adapter digest handler design decision.
Lines 100-104 note that adapters currently have digest handlers, line 109 identifies cloud invocation as a gap, and this section decides not to implement cloud invocation but instead remove the handler requirement. While internally consistent, the rationale for this design pivot could be more explicit.
Optional clarification
Consider adding a brief sentence explaining why generic cloud rendering is preferred over adapter-specific handlers, such as:
- Keep digest rendering generic over workspace events in `cloud`; do not require adapter `digest()` handlers to own provider-specific bullet rendering. Remove the unused adapter-handler expectation from the skill contracts and adapter docs accordingly. + Keep digest rendering generic over workspace events in `cloud` to avoid adapter-specific implementation overhead and maintain consistent digest output format across providers; do not require adapter `digest()` handlers to own provider-specific bullet rendering. Remove the unused adapter-handler expectation from the skill contracts and adapter docs accordingly.🤖 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 `@docs/workspace-primitives-pr39-gap-spec.md` around lines 315 - 318, The docs currently remove the adapter digest() handler requirement and avoid cloud invocation but don't explain why; update the paragraph around the decision (where you mention cloud and removing the adapter-handler expectation from skill contracts and adapter docs) to add one concise sentence that states the rationale: prefer a generic cloud-level rendering of workspace events to keep provider-specific presentation out of adapter digest() handlers, maintain separation of concerns, reduce duplication across adapters, and simplify evolving cloud rendering logic. Ensure the added sentence references digest(), cloud, and the removed adapter-handler expectation so readers can quickly map the rationale to the changed contracts and adapter docs.
🤖 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 @.trajectories/index.json:
- Line 243: Update the "path" values in .trajectories/index.json so they use
relative paths instead of absolute ones: replace occurrences starting with
"/Users/khaliqgant/Projects/AgentWorkforce/relayfile/.trajectories/active/" with
".trajectories/active/" for each affected entry (the JSON "path" field),
ensuring all new entries match the existing relative format used elsewhere in
the file.
In `@cmd/relayfile-cli/digest.go`:
- Around line 173-176: The rebuild branches currently ignore errors from
os.Remove (used with localDir and paths built from digestpkg.YesterdayPath,
digestpkg.YesterdayMarkerPath and digestpkg.DateStampedPath(closedDay, tz)),
which can mask permission/IO failures and leave stale artifacts; update those
calls to capture the returned error, and on failure either return or propagate
an explicit error (e.g., wrap with fmt.Errorf or use processLogger/Errorf and
return) so the rebuild path fails fast instead of silently continuing — do this
for all similar removals (the blocks that call os.Remove for YesterdayPath,
YesterdayMarkerPath and DateStampedPath).
In `@internal/digest/date_stamped.go`:
- Around line 130-145: Replace the current direct O_EXCL create-then-write flow
with a temp-file-then-atomic-publish pattern: create a temp file in the same
directory (e.g. localPath+".tmp"+random), write the content to that temp file,
call f.Sync() and then f.Close(); then attempt to atomically publish the temp
file to the final path by creating a hard link (os.Link(tempPath, localPath))
which will fail if localPath already exists (treat that as Written:false), and
finally remove the temp file; on any error ensure temp is removed and return the
appropriate error. Update the code around the existing localPath, f.Write,
f.Close logic and return values (DateStampedWriteResult, Path, Report, Written)
accordingly so partial writes cannot leave a truncated immutable file.
In `@internal/digest/yesterday_test.go`:
- Around line 278-292: The test builds laterEvents but still calls
WriteYesterday with the original events; update the second WriteYesterday
invocation to pass the modified slice (use SliceSource{Items: laterEvents}) so
the immutability case actually exercises a later-day event being present; make
no other logic changes—keep root, w, and error/assert checks the same.
In `@internal/digest/yesterday.go`:
- Around line 151-157: The current early-return when
readYesterdayMarker(mountRoot) == localDateStr and os.ReadFile(localPath) fails
can hard-fail on a stale marker; change the logic in the w.Closing branch so
that after attempting existing, if os.ReadFile returns an error you check
errors.Is(readErr, os.ErrNotExist) and treat that as a stale marker (i.e., do
not return YesterdayWriteResult{} error but fall through to the normal
render/write flow), while still returning an error for any other readErr; locate
the block referencing w.Closing, readYesterdayMarker, localDateStr,
os.ReadFile(localPath) and YesterdayWriteResult and implement the conditional
handling.
In `@internal/mountfuse/layout.go`:
- Around line 282-343: The providerLayoutManifest path repeatedly rescans via
deriveProviderLayoutManifest; add a memoization cache on fsState (e.g.
map[string]cachedEntry plus sync.Mutex) to store derived manifests keyed by
provider with a TTL and timestamp, check the cache in providerLayoutManifest
before calling deriveProviderLayoutManifest, and update the cache inside
deriveProviderLayoutManifest (or immediately after a successful derive) so
subsequent calls return the cached LayoutManifest until TTL expiry; ensure
thread-safety around reads/writes to the cache and provide a simple invalidation
strategy (TTL or explicit clear) for when configuration or remoteRoot changes.
- Line 138: Update the filename guidance string in the b.WriteString call (in
internal/mountfuse/layout.go) to replace the misleading `<uuid>` token with a
generic `<id>` token so it reads `<identifier>__<id>.json`; keep the rest of the
sentence about recovering the provider id from the last `__`-separated segment
unchanged.
- Around line 177-179: The helper function legacyProviderLayoutRemotePath is
dead code and should be removed to satisfy static analysis; delete the
legacyProviderLayoutRemotePath function from internal/mountfuse/layout.go (it
currently calls joinRemotePath and references legacyProviderLayoutFilename), or
if it was intended to be used, replace its callers to use
legacyProviderLayoutRemotePath instead of duplicating joinRemotePath calls so
the symbol is referenced.
In `@internal/mountsync/syncer.go`:
- Around line 884-886: HandleLocalChange currently calls
s.rollingCoalescer.ObserveChange(relativePath) without holding s.mu while other
code (lines reading/resetting pending/dueAt) accesses that same coalescer state
under s.mu, causing a race; fix by acquiring the same sync lock (s.mu) around
the mutation: wrap the call to rollingCoalescer.ObserveChange in the critical
section (or otherwise ensure s.mu is held) and only access/modify
rollingCoalescer.pending and rollingCoalescer.dueAt while holding s.mu so all
reads and writes to the coalescer state are consistently protected.
---
Nitpick comments:
In @.trajectories/index.json:
- Around line 239-1357: Many short-lived "ricky-child-update-*-workflow"
trajectories were created and abandoned in a tight batch, so locate the
trajectory creation site(s) (look for functions/methods named createTrajectory,
spawnChildTrajectory, TrajectoryManager.create or WorkflowRunner.spawnChild) and
add protections: rate-limit/debounce rapid successive calls, check for existing
active trajectories with the same title/tag before creating a new one (dedupe),
and implement retry/backoff plus clearer logging/error propagation so failures
don't trigger repeated immediate child creations and automatic abandonment.
In `@docs/workspace-primitives-pr39-gap-spec.md`:
- Around line 315-318: The docs currently remove the adapter digest() handler
requirement and avoid cloud invocation but don't explain why; update the
paragraph around the decision (where you mention cloud and removing the
adapter-handler expectation from skill contracts and adapter docs) to add one
concise sentence that states the rationale: prefer a generic cloud-level
rendering of workspace events to keep provider-specific presentation out of
adapter digest() handlers, maintain separation of concerns, reduce duplication
across adapters, and simplify evolving cloud rendering logic. Ensure the added
sentence references digest(), cloud, and the removed adapter-handler expectation
so readers can quickly map the rationale to the changed contracts and adapter
docs.
🪄 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: b0b75613-1979-4afb-b88c-6f4947e85f9e
📒 Files selected for processing (49)
.claude/rules/relayfile-integration-digests.md.trajectories/completed/2026-05/traj_nxbsptr6c5q0.json.trajectories/completed/2026-05/traj_nxbsptr6c5q0.md.trajectories/completed/2026-05/traj_nxbsptr6c5q0.trace.json.trajectories/completed/2026-05/traj_qrt7hh3ht8nk.json.trajectories/completed/2026-05/traj_qrt7hh3ht8nk.md.trajectories/index.jsonAGENTS.mdcmd/relayfile-cli/digest.gocmd/relayfile-cli/digest_test.gocmd/relayfile-cli/main.gocmd/relayfile-mount/main.godocs/workspace-primitives-pr39-gap-spec.mdinternal/digest/date_stamped.gointernal/digest/digest.gointernal/digest/digest_test.gointernal/digest/last_week.gointernal/digest/last_week_test.gointernal/digest/testdata/date-stamped-fixture.mdinternal/digest/testdata/last-week-events.jsonlinternal/digest/testdata/last-week-fixture.mdinternal/digest/testdata/this-week-fixture.mdinternal/digest/testdata/today-fixture.mdinternal/digest/this_week.gointernal/digest/this_week_test.gointernal/digest/timezone.gointernal/digest/timezone_test.gointernal/digest/today.gointernal/digest/today_test.gointernal/digest/types.gointernal/digest/yesterday.gointernal/digest/yesterday_test.gointernal/mountfuse/fs.gointernal/mountfuse/fuse_test.gointernal/mountfuse/layout.gointernal/mountfuse/layout_test.gointernal/mountsync/digest_filter_test.gointernal/mountsync/digest_writer_test.gointernal/mountsync/scheduler.gointernal/mountsync/scheduler_test.gointernal/mountsync/syncer.gointernal/mountsync/syncer_test.gointernal/mountsync/watcher_test.gopackages/sdk/python/README.mdpackages/sdk/python/src/relayfile/__init__.pypackages/sdk/python/src/relayfile/client.pypackages/sdk/python/src/relayfile/provider.pypackages/sdk/python/src/relayfile/types.pypackages/sdk/python/tests/test_provider.py
💤 Files with no reviewable changes (5)
- .trajectories/completed/2026-05/traj_nxbsptr6c5q0.md
- .trajectories/completed/2026-05/traj_qrt7hh3ht8nk.md
- .trajectories/completed/2026-05/traj_nxbsptr6c5q0.json
- .trajectories/completed/2026-05/traj_qrt7hh3ht8nk.json
- .trajectories/completed/2026-05/traj_nxbsptr6c5q0.trace.json
Comment on lines
+130
to
+145
| f, err := os.OpenFile(localPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) | ||
| if errors.Is(err, os.ErrExist) { | ||
| return DateStampedWriteResult{Path: path, Report: rep, Written: false}, nil | ||
| } | ||
| if err != nil { | ||
| return DateStampedWriteResult{}, err | ||
| } | ||
| if _, err := f.Write(content); err != nil { | ||
| _ = f.Close() | ||
| _ = os.Remove(localPath) | ||
| return DateStampedWriteResult{}, err | ||
| } | ||
| if err := f.Close(); err != nil { | ||
| _ = os.Remove(localPath) | ||
| return DateStampedWriteResult{}, err | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current write strategy in WriteDateStamped (direct final-path O_EXCL write).
rg -n -A35 -B10 'func WriteDateStamped|O_EXCL|CreateTemp|Rename|Link' internal/digest/date_stamped.goRepository: AgentWorkforce/relayfile
Length of output: 3194
🏁 Script executed:
# Check for callers of WriteDateStamped and MaybeCloseDateStampedWindow
rg -n 'WriteDateStamped|MaybeCloseDateStampedWindow' --type go -B2 -A2 | grep -E '(func|WriteDateStamped|MaybeCloseDateStampedWindow)' | head -50Repository: AgentWorkforce/relayfile
Length of output: 3753
🏁 Script executed:
# Verify if Sync() is called anywhere in the file
rg -n 'Sync|sync|fsync' internal/digest/date_stamped.goRepository: AgentWorkforce/relayfile
Length of output: 145
🏁 Script executed:
# Check the full file structure to see if there's any surrounding context
wc -l internal/digest/date_stamped.goRepository: AgentWorkforce/relayfile
Length of output: 104
Use atomic file creation (temp+rename) instead of direct write with O_EXCL.
Process crash between f.Write() (line 137) and f.Close() (line 142) leaves a truncated file on disk. Because the file then exists, the O_EXCL check at line 131 prevents repair—subsequent runs skip creation, leaving a corrupted digest permanent. This is especially risky given the scheduler calls this function in a clock-driven loop (scheduler.go:127).
Write to a temporary file, fsync it, then atomically move it to the final path only if the target does not exist. This ensures partial writes never become the immutable artifact.
🤖 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 `@internal/digest/date_stamped.go` around lines 130 - 145, Replace the current
direct O_EXCL create-then-write flow with a temp-file-then-atomic-publish
pattern: create a temp file in the same directory (e.g.
localPath+".tmp"+random), write the content to that temp file, call f.Sync() and
then f.Close(); then attempt to atomically publish the temp file to the final
path by creating a hard link (os.Link(tempPath, localPath)) which will fail if
localPath already exists (treat that as Written:false), and finally remove the
temp file; on any error ensure temp is removed and return the appropriate error.
Update the code around the existing localPath, f.Write, f.Close logic and return
values (DateStampedWriteResult, Path, Report, Written) accordingly so partial
writes cannot leave a truncated immutable file.
Comment on lines
+282
to
+343
| func (s *fsState) providerLayoutManifest(ctx context.Context, provider string) LayoutManifest { | ||
| provider = strings.TrimSpace(provider) | ||
| if manifest, ok := s.configuredLayoutManifest(provider); ok { | ||
| return manifest | ||
| } | ||
| manifest, err := s.deriveProviderLayoutManifest(ctx, provider) | ||
| if err != nil { | ||
| s.logf("derive provider layout for %s: %v", provider, err) | ||
| return LayoutManifest{Provider: provider} | ||
| } | ||
| return manifest | ||
| } | ||
|
|
||
| func (s *fsState) deriveProviderLayoutManifest(ctx context.Context, provider string) (LayoutManifest, error) { | ||
| manifest := LayoutManifest{ | ||
| Provider: strings.TrimSpace(provider), | ||
| LazyMaterialization: s.lazyRepos != nil && strings.TrimSpace(provider) == "github", | ||
| } | ||
| if manifest.Provider == "" || s.client == nil { | ||
| return manifest, nil | ||
| } | ||
|
|
||
| resources := map[string]struct{}{} | ||
| aliases := map[string]struct{}{} | ||
| providerRoot := joinRemotePath(s.remoteRoot, manifest.Provider) | ||
| cursor := "" | ||
| for { | ||
| resp, err := s.client.ListTree(ctx, s.workspaceID, providerRoot, providerLayoutDeriveDepth, cursor) | ||
| if err != nil { | ||
| return manifest, err | ||
| } | ||
| for _, entry := range resp.Entries { | ||
| rel, ok := relativeToRemoteRoot(providerRoot, entry.Path) | ||
| if !ok || rel == "" { | ||
| continue | ||
| } | ||
| segments := providerLayoutPathSegments(rel) | ||
| if len(segments) == 0 { | ||
| continue | ||
| } | ||
| if providerLayoutResourceSegment(segments[0]) != "" && (len(segments) > 1 || isTreeDirectory(entry.Type)) { | ||
| resources[segments[0]] = struct{}{} | ||
| } | ||
| for _, segment := range segments[1:] { | ||
| if isProviderLayoutAliasSegment(segment) { | ||
| aliases[segment] = struct{}{} | ||
| } | ||
| } | ||
| } | ||
| if resp.NextCursor == nil || strings.TrimSpace(*resp.NextCursor) == "" { | ||
| break | ||
| } | ||
| cursor = strings.TrimSpace(*resp.NextCursor) | ||
| } | ||
|
|
||
| for resource := range resources { | ||
| manifest.ResourceDirectories = append(manifest.ResourceDirectories, resource) | ||
| } | ||
| for alias := range aliases { | ||
| manifest.AliasSegments = append(manifest.AliasSegments, alias) | ||
| } | ||
| return normalizeLayoutManifest(manifest), nil |
There was a problem hiding this comment.
Cache derived provider manifests instead of rescanning on every access.
For providers without injected manifests, this path re-runs ListTree derivation each time providerLayoutManifest is called. Since layout metadata/content lookups call into this repeatedly, /provider/LAYOUT.md reads can trigger repeated depth-4 tree scans and transiently inconsistent output under backend flakiness.
Please memoize derived manifests (with invalidation/TTL) so provider layout reads are cheap and stable.
🤖 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 `@internal/mountfuse/layout.go` around lines 282 - 343, The
providerLayoutManifest path repeatedly rescans via deriveProviderLayoutManifest;
add a memoization cache on fsState (e.g. map[string]cachedEntry plus sync.Mutex)
to store derived manifests keyed by provider with a TTL and timestamp, check the
cache in providerLayoutManifest before calling deriveProviderLayoutManifest, and
update the cache inside deriveProviderLayoutManifest (or immediately after a
successful derive) so subsequent calls return the cached LayoutManifest until
TTL expiry; ensure thread-safety around reads/writes to the cache and provide a
simple invalidation strategy (TTL or explicit clear) for when configuration or
remoteRoot changes.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 49 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/mountsync/scheduler.go">
<violation number="1" location="internal/mountsync/scheduler.go:61">
P2: Debounce without a max-delay cap can starve rolling digest regeneration indefinitely. If filesystem events arrive faster than the 30s interval, `dueAt` is perpetually pushed forward and `Due()` never returns `true`. Consider tracking the time of the first un-flushed event and capping the maximum delay (e.g., 5 minutes) so digests remain current under sustained write pressure.</violation>
</file>
<file name="internal/digest/timezone.go">
<violation number="1" location="internal/digest/timezone.go:28">
P2: The original error from `time.LoadLocation` is discarded. Include it in the wrapped error so callers can distinguish between an unknown zone name and a missing tzdata file.</violation>
</file>
<file name=".trajectories/index.json">
<violation number="1" location=".trajectories/index.json:243">
P2: All new trajectory entries use absolute local paths (`/Users/khaliqgant/Projects/...`) instead of the relative paths (`.trajectories/...`) used by every existing entry. These machine-specific paths will break on any other developer's machine or in CI and violate the established convention in this file.</violation>
</file>
<file name="internal/digest/today.go">
<violation number="1" location="internal/digest/today.go:160">
P2: The temp file is renamed into place without setting file permissions. `os.CreateTemp` creates files with mode 0600. The codebase convention (see `writeFileAtomic` in mountsync) is to `Chmod(0o644)` before rename so digest artifacts are readable by other processes. Consider adding `tmp.Chmod(0o644)` before `tmp.Close()`.</violation>
</file>
<file name="internal/digest/yesterday_test.go">
<violation number="1" location="internal/digest/yesterday_test.go:162">
P2: `laterEvents` is constructed but never used. The second `WriteYesterday` call passes the original `events` instead of `laterEvents`, so this test doesn't actually verify immutability when later-day events are present in the source — it just re-tests the byte-equal idempotency path. If the intent is to test that the writer is resilient to a source containing events outside the closed day, pass `laterEvents` here.</violation>
</file>
<file name="internal/mountsync/syncer.go">
<violation number="1" location="internal/mountsync/syncer.go:884">
P1: Data race: `ObserveChange` is called without holding `s.mu`, while `Due()` and `MarkFlushed()` are called under `s.mu`. Since `RollingDigestCoalescer` has no internal synchronization, concurrent access to `c.pending` and `c.dueAt` is a data race. Move the `ObserveChange` call below the `s.mu.Lock()` to use the syncer's existing mutex as the synchronization point.</violation>
</file>
<file name="cmd/relayfile-cli/digest.go">
<violation number="1" location="cmd/relayfile-cli/digest.go:252">
P2: The `digests` directory is not skipped with `filepath.SkipDir` in the walk, unlike in `collectDigestProviders`. The walker descends into `digests/` and checks each file individually via `IsDigestPath`. Add `"digests"` to the directory skip switch to avoid traversing accumulated digest files unnecessarily.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
…obustness - CRITICAL data race: RollingDigestCoalescer.ObserveChange ran before s.mu.Lock() while Due()/MarkFlushed() run under s.mu. Moved under the lock; documented the single-sync-point contract. `go test -race` clean. - MAJOR coalescer starvation: debounce reset dueAt every change so it never fired under sustained sub-interval churn. Added MaxDelay cap (default 5*interval) anchored at firstPendingAt + regression test. - MAJOR portability: 204 absolute .trajectories paths normalized to relative to match the existing convention. - today.go/this_week.go: Chmod 0644 before rename (CreateTemp is 0600) so digests are readable by mount/agents. - timezone.go: wrap original LoadLocation error. - yesterday.go: stale-marker robustness — regenerate on ErrNotExist instead of wedging the close pipeline. - yesterday_test.go: pass laterEvents so the test actually exercises marker-pinned immutability against a contaminated source. - cmd digest rebuild: removeForRebuild surfaces non-ErrNotExist delete failures so a stale marker can't make a forced rebuild a silent no-op. - digest.go Events walk: SkipDir on digests/. - mountfuse/layout.go: `<id>` wording; removed dead helper. go build/vet/test ./... green; -race clean; contract + pytest pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Addressed the review in b5f6fe3. Verified: Fixed
Deferred, with reasoning (per "skip with a brief reason, keep changes minimal")
|
Reconciles PR #162 (digest runtime) with the mount-root clobber hardening that landed on main (#163 clawpatch findings, #164 defense-in-depth, #165 structural mount-root invariants). Only conflict was internal/mountsync/syncer.go — disjoint struct additions: union of #162's `closeScheduler`/`rollingCoalescer` (digest scheduler + coalescer) and main's `circuit *CloudErrorCircuit` (cloud-error breaker), plus the matching constructor initializers. No behavioral overlap. Verified: go build ./... clean; go test ./... all green; go test -race ./internal/mountsync clean (the #162 ObserveChange data- race fix holds with main's mount-root hardening present); scripts/check-contract-surface.sh passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Status update — all inline review comments were already addressed. The CodeRabbit/cubic/Devin inline threads above were posted before commit
Two heavy-lift items remain intentionally deferred with rationale (documented earlier in this PR): Branch is now merged current with |
khaliqgant
pushed a commit
that referenced
this pull request
May 19, 2026
…obustness - CRITICAL data race: RollingDigestCoalescer.ObserveChange ran before s.mu.Lock() while Due()/MarkFlushed() run under s.mu. Moved under the lock; documented the single-sync-point contract. `go test -race` clean. - MAJOR coalescer starvation: debounce reset dueAt every change so it never fired under sustained sub-interval churn. Added MaxDelay cap (default 5*interval) anchored at firstPendingAt + regression test. - MAJOR portability: 204 absolute .trajectories paths normalized to relative to match the existing convention. - today.go/this_week.go: Chmod 0644 before rename (CreateTemp is 0600) so digests are readable by mount/agents. - timezone.go: wrap original LoadLocation error. - yesterday.go: stale-marker robustness — regenerate on ErrNotExist instead of wedging the close pipeline. - yesterday_test.go: pass laterEvents so the test actually exercises marker-pinned immutability against a contaminated source. - cmd digest rebuild: removeForRebuild surfaces non-ErrNotExist delete failures so a stale marker can't make a forced rebuild a silent no-op. - digest.go Events walk: SkipDir on digests/. - mountfuse/layout.go: `<id>` wording; removed dead helper. go build/vet/test ./... green; -race clean; contract + pytest pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> # Conflicts: # .trajectories/index.json # internal/mountsync/syncer.go
Member
Author
khaliqgant
added a commit
that referenced
this pull request
May 19, 2026
…167) * feat(workspace): apply cleaned PR-39 local primitives * feat(digest): PR-39 workspace-primitives digest runtime Implements the digest/activity-summary gaps from docs/workspace-primitives-pr39-gap-spec.md: - internal/digest: date-stamped (YYYY-MM-DD), this-week, last-week, today, yesterday window builders with workspace-timezone support and golden fixtures. - cmd/relayfile-cli: `relayfile digest rebuild --window ...` real implementation (was a stub) plus tests. - internal/mountfuse: virtual LAYOUT.md / digest exposure in the FUSE layer. - internal/mountsync: digest scheduler, digest-source event filtering (no recursion on /digests/* writes), digest writer + watcher tests. - packages/sdk/python: expanded digest path taxonomy. - .claude/rules/relayfile-integration-digests.md: digest runtime contract rule. - docs/workspace-primitives-pr39-gap-spec.md: the gap spec this implements, with resolved decisions. Generated by a Ricky master-executor run (12 child slices); only the product source/tests/spec are committed here. Runtime digest output (digests/*.md), ricky scratch (.workflow-artifacts/, workflows/generated/, internal/digest/cmd/ demos, mount-verify/), and trail bookkeeping are intentionally excluded. Verified: go build ./... clean; go test ./internal/digest ./internal/mountfuse ./internal/mountsync ./cmd/relayfile-cli all pass; pytest packages/sdk/python 9 passed; scripts/check-contract-surface.sh passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> # Conflicts: # cmd/relayfile-cli/digest.go # cmd/relayfile-cli/digest_test.go # cmd/relayfile-cli/main.go # internal/mountsync/syncer.go * fix(digest): address PR #162 review — data race, starvation, perms, robustness - CRITICAL data race: RollingDigestCoalescer.ObserveChange ran before s.mu.Lock() while Due()/MarkFlushed() run under s.mu. Moved under the lock; documented the single-sync-point contract. `go test -race` clean. - MAJOR coalescer starvation: debounce reset dueAt every change so it never fired under sustained sub-interval churn. Added MaxDelay cap (default 5*interval) anchored at firstPendingAt + regression test. - MAJOR portability: 204 absolute .trajectories paths normalized to relative to match the existing convention. - today.go/this_week.go: Chmod 0644 before rename (CreateTemp is 0600) so digests are readable by mount/agents. - timezone.go: wrap original LoadLocation error. - yesterday.go: stale-marker robustness — regenerate on ErrNotExist instead of wedging the close pipeline. - yesterday_test.go: pass laterEvents so the test actually exercises marker-pinned immutability against a contaminated source. - cmd digest rebuild: removeForRebuild surfaces non-ErrNotExist delete failures so a stale marker can't make a forced rebuild a silent no-op. - digest.go Events walk: SkipDir on digests/. - mountfuse/layout.go: `<id>` wording; removed dead helper. go build/vet/test ./... green; -race clean; contract + pytest pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> # Conflicts: # .trajectories/index.json # internal/mountsync/syncer.go --------- Co-authored-by: Proactive Runtime Bot <agent@agent-relay.com>
Member
Author
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
The relayfile-owned slice of
docs/workspace-primitives-pr39-gap-spec.md. Implements the local/self-host digest runtime, CLI rebuild, mount exposure, and SDK taxonomy. Verified end-to-end locally (built CLI, exercised all 5 windows, full Go suite green).Delivered (relayfile scope)
today,yesterday,YYYY-MM-DD,this-week,last-weekwindow builders; workspace-timezone support (ResolveTZ, DST boundaries); immutable closing windows; exhaustive coverage (no 500-cap; cap retained only for back-compat); golden fixtures per window.relayfile digest rebuild --window today|yesterday|this-week|last-week|YYYY-MM-DD [--workspace] [--json]— was a stub, now real; reports regenerated path + event count (spec acceptance check).by-edited/<date>/alias segment in mountfuse layout + mountsync.relayfile writeback list --state pending|dead|succeeded|failed.LAYOUT.mdcanonicalized across runtime/fixtures/skill contract (legacy.layout.mdcompat retained); digest runtime contract rule (.claude/rules/relayfile-integration-digests.md+ CLAUDE.md).Verification
go build ./...cleango test ./...— all packages green (75+ digest tests: immutability, tz/DST, exhaustive/over-cap, header contract)relayfile digest rebuildexercised for all 5 windows (path + event count reported)pytest packages/sdk/python9 passed ·scripts/check-contract-surface.shpassedOut of scope (NOT in this PR)
The spec is 3-repo. This PR is the relayfile slice only.
cloud(hosted digest scheduler, adapterdigest()invocation, hosted timezone, materialize/.skills/activity-summary.md) andrelayfile-adapters(provider digest handler conformance, adapter-sideby-edited) are not implemented here — tracked as follow-up runs scoped to those repos.Note on branch contents
digest-tighteningalso carries two pre-existing, unrelated commits that were on the branch before this work:ba9c28c fix(mount): avoid false low-memory pending stateandc93ea56 fix(mount): add low-memory sync diagnostics. Flagging for reviewer awareness — split out if you'd rather they land separately.🤖 Generated with Claude Code
Merge conflict resolution
origin/mainintodigest-tighteningto resolve the dirty PR state.internal/mountsync/syncer.goby keeping the digest close scheduler / rolling digest coalescer setup from this PR and the provider-layout observed alias handling, while retaining the currentmainpolling and release changes.go test ./internal/mountsyncandgo test ./...using Go 1.22.12.Initiative Context