feat: human-readable job ids (title-HH:MM format)#9
Merged
Conversation
- New src/jobs/id.rs: `JobId(String)` matching
`^[a-z0-9-]+-\d{2}:\d{2}(-\d+)?$` — e.g. `cargo-build-23:30`,
replacing opaque counter ids with conversation-friendly ones.
- slug(): first 3 cmd tokens, lowercased, non-alnum→`-`, collapsed,
trimmed, capped at 24; falls back to `job` when nothing survives.
- generate(cmd, &AtomicU64, exists): clean base id; appends `-{seq}`
only on collision. Takes an `exists` predicate (vs. bare signature)
so collision detection lives in this module and stays decoupled from
the store's id map — required for the collision unit test.
- chrono (clock, no default features) for `Local` HH:MM.
- 10 unit tests: slug rules, format regex shape, collision suffix,
Display/AsRef, transparent serialize.
- #[allow(dead_code)] until the next change wires JobId into JobStore.
Co-Authored-By: Claude <noreply@anthropic.com>
- run() mints `slug-HH:MM` ids via id::generate under the jobs lock, so concurrent same-minute commands reserve unique ids without clobbering. - RunResult::Backgrounded, JobSummary, the jobs map, and the reaper all key on JobId; poll/kill take &JobId, wrapped at the tools edge from client input. - JobId gains Borrow<str> (O(1) candidate probe), From<String>/<&str> (lookup wrapping), Ord (list sort); drop the staging dead_code allow. Co-Authored-By: Claude <noreply@anthropic.com>
Move Page struct, DEFAULT_PAGE const, read_page fn, and their test into a dedicated src/jobs/log.rs; re-export Page from mod.rs. Co-Authored-By: Claude <noreply@anthropic.com>
…o-build-23:30) Updated docs to reflect the new human-readable job id format (command slug + local hour): - docs/usage.md: replaced j1,j2,...,j7 examples with cargo-build-23:30 - docs/architecture.md: updated module map to show jobs/id.rs, jobs/log.rs, jobs/reaper.rs separately (was a single jobs.rs); noted id format gives context without UUIDs - CLAUDE.md: added module map entries for jobs/id.rs and jobs/log.rs Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 23 minutes and 34 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds human-readable ChangesJob ID and log flow
Sequence Diagram(s)sequenceDiagram
participant Tool as job tool
participant Store as JobStore
participant Reader as read_page
Tool->>Store: poll(JobId, cursor, limit)
Store->>Reader: read_page(path, cursor, limit)
Reader-->>Store: Page { lines, next_cursor, total_lines, has_more }
Store-->>Tool: JobState + Page
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 7
🤖 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 `@CLAUDE.md`:
- Line 57: The module-map entry for JobId is imprecise: `JobId::generate` uses
`%H:%M`, so “local hour” understates the actual timestamp format. Update the
`src/jobs/id.rs` description in `CLAUDE.md` to say “local time” or “local
hour:minute” so it matches the `JobId::generate` behavior and avoids confusion.
In `@docs/architecture.md`:
- Around line 25-28: The architecture doc is inconsistent because the
request-flow diagram still references the old job/file modules while the table
now maps the flow to jobs/mod.rs and jobs/reaper.rs. Update the diagram in
docs/architecture.md to match the new module names and routing, using the same
symbols already listed in the table (jobs/mod.rs, jobs/id.rs, jobs/log.rs,
jobs/reaper.rs) so the diagram and table describe the same code path.
In `@docs/usage.md`:
- Line 16: The fenced code block in the documentation is missing a language tag
and is triggering markdownlint MD040. Update the new fenced block to use an
explicit text language identifier, and make sure the surrounding markdown in
docs/usage.md remains valid and lint-clean.
- Around line 20-29: The polling example’s cursor progression is inconsistent:
after the first poll shows next_cursor=200, the next job(action="poll") example
should not jump directly to cursor=540 without explaining a larger fetch size.
Update the job(action="poll") sequence in the usage example so it either
includes the missing intermediate next_cursor=400 page, or clearly annotates the
second follow-up call to show it uses an explicit larger limit; keep the
progression consistent with the polling behavior shown in the same snippet.
In `@src/jobs/log.rs`:
- Line 23: The page-end calculation in the pagination logic can overflow when
`cursor` and `limit` are large, so update the `end` computation in
`src/jobs/log.rs` to use saturating addition instead of plain `cursor + limit`;
locate the fix near the existing `cursor`, `limit`, and `total` slice-window
logic and ensure the window is still capped with `min(total)` after the
saturating add.
- Around line 16-20: The log page reader in read_page currently hides file read
failures by defaulting to empty bytes, which makes missing or unreadable logs
indistinguishable from a real empty log. Change read_page to return a typed
domain error instead of always producing a Page, and propagate the
tokio::fs::read failure with a thiserror-based error type. Then update the
job(action="poll") caller to handle and surface that error rather than
converting it into an empty result.
In `@src/jobs/mod.rs`:
- Around line 94-100: `JobId::generate(&cmd, ...)` is deriving job IDs from raw
shell text, which can leak secrets through `bash`, job listing, logs, and
filenames. Update the `jobs`-lock section in `src/jobs/mod.rs` to generate the
`JobId` from a neutral prefix or explicit safe label instead of `cmd`, and keep
the collision check against `jobs.contains_key(...)` so IDs remain unique
without exposing command tokens.
🪄 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
Run ID: c0943afa-17e7-467c-9232-4f9777a408f2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
CLAUDE.mdCargo.tomldocs/architecture.mddocs/usage.mdsrc/jobs/id.rssrc/jobs/log.rssrc/jobs/mod.rssrc/jobs/reaper.rssrc/tools/mod.rs
- security: derive JobId from a neutral "job" label, never command text; a secret in leading tokens (mysql -psecret, PGPASSWORD=…) no longer leaks into ids, reaper logs, or log filenames - jobs/log: read_page returns Result<Page, JobLogError> (thiserror) so a missing/unreadable log surfaces as an error instead of an empty page; poll/run/tools wired through, run converts at its io::Result boundary - jobs/log: saturating_add for the page end to avoid cursor+limit overflow - docs: sync architecture diagram to jobs/mod.rs + tools/files.rs; fix usage poll-cursor progression (200→400→exit); tag fenced block (MD040); precise id description (local time, neutral prefix) in CLAUDE/architecture Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/architecture.md`:
- Line 62: The documentation is still claiming that the full command is returned
via job(action="list"), which exposes secrets despite the sanitized job id.
Update the behavior described around job(action="list") and the in-memory job
listing to redact or omit the cmd field from public listings, and make sure any
references to Job ids, the hourly reaper, and the job dir clearly reflect that
only non-sensitive metadata is exposed.
In `@src/jobs/id.rs`:
- Around line 25-32: The JobId constructor still accepts an arbitrary label,
which preserves the possibility of leaking command text into ids. Update
JobId::generate so it owns the fixed neutral prefix internally (use the
hard-coded "job" prefix) and remove the caller-supplied label from the API; keep
the existing seq/existing-check behavior and preserve the local time suffix
formatting.
In `@src/jobs/log.rs`:
- Around line 39-62: The pagination helper in log reading must not allow limit=0
to produce a non-advancing page. Update the function that builds Page in
src/jobs/log.rs (the read_page-style logic using cursor, limit, next_cursor, and
has_more) to either reject zero limits up front or clamp the limit to at least 1
before computing the window. Also add a regression test covering read_page(path,
cursor, 0) to verify it does not return an infinite-loop page.
In `@src/jobs/mod.rs`:
- Around line 183-197: The Jobs::poll path currently turns a race with
reaper::reap_once into a JobLogError::Read(NotFound) even though the job was
just evicted and should still look unknown to callers. In poll, after
read_page(&job.log_path, ...) fails with NotFound, re-check jobs.lock().await
for the id; if it is no longer present, return Ok(None) instead of propagating
the error. Keep the existing JobId lookup, JobState clone, and Page read flow
intact, but special-case the filesystem NotFound from read_page to map back to
the stable unknown-job result.
🪄 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
Run ID: d1d47ed4-f0b0-4322-8294-10aa4e7a7c18
📒 Files selected for processing (7)
CLAUDE.mddocs/architecture.mddocs/usage.mdsrc/jobs/id.rssrc/jobs/log.rssrc/jobs/mod.rssrc/tools/mod.rs
- Omit `cmd` from JobSummary/Job — job(list) no longer returns raw commands (could carry PGPASSWORD=…/bearer tokens) in MCP responses - Hard-code the `job` prefix in JobId::generate; drop the label param and slug() so command text can never reach an id - Clamp read_page limit to >=1 so limit=0 can't return a non-advancing page (client paging on next_cursor would spin) - Map reaper eviction races in poll (NotFound + id gone) to Ok(None) instead of surfacing a filesystem error - Update architecture.md: list exposes only id + status Co-Authored-By: Claude <noreply@anthropic.com>
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
Introduced human-readable job IDs replacing random UUIDs. Job IDs now follow the format
command-slug-HH:MM(e.g.,cargo-build-23:30), providing context about what the job does and when it was created.cargo-build-23:30style; module map reflects the SRP splitQuality
clippy -D warningscleanbash/job/file) stays constant🎯 TASK COMPLETE
Summary by CodeRabbit