Repository navigation
feat: human-readable job ids (title-HH:MM format) #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
8be9421
feat(jobs): JobId newtype with slug-from-cmd + local HH:MM
sebyx07 9901ea7
feat(jobs): wire JobId through the engine
sebyx07 a1a9944
refactor(jobs): extract Page + read_page into jobs/log.rs
sebyx07 7444d55
docs: replace job id examples j1..j7 with human-readable format (carg…
sebyx07 ff2b58a
fix(jobs): address PR #9 review — secret-safe ids, typed log errors
sebyx07 7e1397b
fix(jobs): close command-text leak surfaces, harden pagination
sebyx07 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,146 @@ | ||
| //! Human-readable job ids: a neutral `job` prefix plus the local `HH:MM` it | ||
| //! started, e.g. `job-23:30` — easier to refer to in conversation than an opaque | ||
| //! counter. The prefix is hard-coded, never derived from the command: a command | ||
| //! can carry a secret in its leading tokens, and the id surfaces in replies, | ||
| //! `job(list)`, reaper logs, and the log filename, so the constructor refuses to | ||
| //! accept command text at all. A monotonic sequence suffix disambiguates jobs | ||
| //! starting within the same minute. | ||
|
|
||
| use std::borrow::Borrow; | ||
| use std::fmt; | ||
| use std::sync::atomic::{AtomicU64, Ordering}; | ||
|
|
||
| use chrono::Local; | ||
|
|
||
| /// A human-readable job identifier: `job-<HH:MM>` with an optional `-<seq>` | ||
| /// collision suffix. A generated id matches `^job-\d{2}:\d{2}(-\d+)?$`; | ||
| /// ids wrapped from client input for lookup are not revalidated. | ||
| #[derive(Debug, Clone, Hash, Eq, PartialEq, PartialOrd, Ord, serde::Serialize)] | ||
| pub struct JobId(String); | ||
|
|
||
| impl JobId { | ||
| /// Build an id from the hard-coded `job` prefix and the current local time. | ||
| /// The prefix is fixed here, never caller-supplied, so command text — which | ||
| /// can carry a secret in its leading tokens — can't leak into the id. `exists` | ||
| /// reports whether a candidate is already taken; only on a clash do we append | ||
| /// a `-<seq>` drawn from `seq`, so clean ids stay clean. The predicate keeps | ||
| /// this decoupled from however the caller tracks live ids. | ||
| pub fn generate(seq: &AtomicU64, exists: impl Fn(&str) -> bool) -> Self { | ||
| let base = format!("job-{}", Local::now().format("%H:%M")); | ||
| if !exists(&base) { | ||
| return Self(base); | ||
| } | ||
| // Same minute: a monotonic suffix keeps the id unique without sacrificing | ||
| // readability. | ||
| let n = seq.fetch_add(1, Ordering::Relaxed); | ||
| Self(format!("{base}-{n}")) | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for JobId { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| f.write_str(&self.0) | ||
| } | ||
| } | ||
|
|
||
| impl AsRef<str> for JobId { | ||
| fn as_ref(&self) -> &str { | ||
| &self.0 | ||
| } | ||
| } | ||
|
|
||
| /// Lets a `HashMap<JobId, _>` be probed with a `&str` key — used when generating | ||
| /// an id to test a candidate against the live ids. | ||
| impl Borrow<str> for JobId { | ||
| fn borrow(&self) -> &str { | ||
| &self.0 | ||
| } | ||
| } | ||
|
|
||
| /// Wrap a client-supplied id for lookup. Not validated — an unknown or malformed | ||
| /// id simply matches no job; only `generate` mints well-formed ids. | ||
| impl From<String> for JobId { | ||
| fn from(s: String) -> Self { | ||
| Self(s) | ||
| } | ||
| } | ||
|
|
||
| impl From<&str> for JobId { | ||
| fn from(s: &str) -> Self { | ||
| Self(s.to_owned()) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| /// Mirror the documented `^job-\d{2}:\d{2}(-\d+)?$` shape without pulling in a | ||
| /// regex dependency. The id has exactly one `:` (prefix and suffix never | ||
| /// contain one), so split on it to isolate the time. | ||
| fn valid_format(s: &str) -> bool { | ||
| let two_digits = |x: &str| x.len() == 2 && x.bytes().all(|b| b.is_ascii_digit()); | ||
| let Some((left, right)) = s.split_once(':') else { | ||
| return false; | ||
| }; | ||
| let Some((prefix, hh)) = left.rsplit_once('-') else { | ||
| return false; | ||
| }; | ||
| let (mm, suffix_ok) = match right.split_once('-') { | ||
| Some((mm, sfx)) => ( | ||
| mm, | ||
| !sfx.is_empty() && sfx.bytes().all(|b| b.is_ascii_digit()), | ||
| ), | ||
| None => (right, true), | ||
| }; | ||
| two_digits(hh) && two_digits(mm) && suffix_ok && prefix == "job" | ||
| } | ||
|
|
||
| #[test] | ||
| fn generated_id_matches_documented_format() { | ||
| let seq = AtomicU64::new(1); | ||
| let id = JobId::generate(&seq, |_| false); | ||
| assert!(valid_format(id.as_ref()), "bad format: {}", id.as_ref()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn no_collision_leaves_id_clean_and_seq_untouched() { | ||
| let seq = AtomicU64::new(1); | ||
| let id = JobId::generate(&seq, |_| false); | ||
| let s = id.as_ref(); | ||
| assert!(valid_format(s)); | ||
| let (_, right) = s.split_once(':').expect("id carries a time"); | ||
| assert!(!right.contains('-'), "unexpected collision suffix: {s}"); | ||
| assert_eq!( | ||
| seq.load(Ordering::Relaxed), | ||
| 1, | ||
| "seq advanced without a clash" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn collision_appends_sequence_suffix() { | ||
| let seq = AtomicU64::new(1); | ||
| // `exists` always true => the base is "taken", forcing the suffix path. | ||
| let id = JobId::generate(&seq, |_| true); | ||
| let s = id.as_ref(); | ||
| assert!(valid_format(s)); | ||
| assert!(s.ends_with("-1"), "expected `-1` seq suffix, got {s}"); | ||
| assert_eq!(seq.load(Ordering::Relaxed), 2, "clash must advance seq"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn display_and_as_ref_agree() { | ||
| let seq = AtomicU64::new(1); | ||
| let id = JobId::generate(&seq, |_| false); | ||
| assert_eq!(id.to_string(), id.as_ref()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn serializes_as_a_plain_string() { | ||
| let seq = AtomicU64::new(1); | ||
| let id = JobId::generate(&seq, |_| false); | ||
| let json = serde_json::to_string(&id).expect("serialize"); | ||
| assert_eq!(json, format!("\"{}\"", id.as_ref())); | ||
| } | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| //! Paginated log reading for per-job output files. | ||
|
|
||
| /// One page of log lines plus a cursor to fetch the next page. | ||
| #[derive(Debug, serde::Serialize)] | ||
| pub struct Page { | ||
| pub lines: Vec<String>, | ||
| pub next_cursor: usize, | ||
| pub total_lines: usize, | ||
| pub has_more: bool, | ||
| } | ||
|
|
||
| pub const DEFAULT_PAGE: usize = 200; | ||
|
|
||
| /// A job log could not be read. Kept distinct from a legitimately empty log so | ||
| /// `job(action="poll")` can surface "log unavailable" instead of silently | ||
| /// returning an empty page (a missing or unreadable log must not look identical | ||
| /// to one that simply has no output yet). | ||
| #[derive(Debug, thiserror::Error)] | ||
| pub enum JobLogError { | ||
| #[error("reading job log: {0}")] | ||
| Read(#[from] std::io::Error), | ||
| } | ||
|
|
||
| /// Convert back to `io::Error` at the `JobStore::run` boundary, which already | ||
| /// reports in `std::io::Result`. The typed error matters for `poll`, which | ||
| /// surfaces it to the caller rather than collapsing it into an empty page. | ||
| impl From<JobLogError> for std::io::Error { | ||
| fn from(e: JobLogError) -> Self { | ||
| match e { | ||
| JobLogError::Read(io) => io, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Read lines `[cursor, cursor+limit)` from a log file. Re-reads the whole file | ||
| /// each call — fine for typical logs; seek by byte offset if they get huge. | ||
| pub async fn read_page( | ||
| path: &std::path::Path, | ||
| cursor: usize, | ||
| limit: usize, | ||
| ) -> Result<Page, JobLogError> { | ||
| // A zero limit would yield an empty page whose `next_cursor == cursor` with | ||
| // `has_more` still true, so a client advancing on `next_cursor` would spin | ||
| // forever on the same window. Clamp to at least one line. | ||
| let limit = limit.max(1); | ||
| // A failed read (missing/unreadable log) propagates as a typed error. Only | ||
| // non-UTF-8 *content* is rendered lossily: a command that writes raw bytes | ||
| // (compiled output, escape sequences) must not produce a silently-empty page. | ||
| let bytes = tokio::fs::read(path).await?; | ||
| let content = String::from_utf8_lossy(&bytes); | ||
| let all: Vec<&str> = content.lines().collect(); | ||
| let total = all.len(); | ||
| // cursor + limit come from tool input; saturate so an oversized request can't | ||
| // overflow into a bogus window. | ||
| let end = cursor.saturating_add(limit).min(total); | ||
| let lines = all | ||
| .get(cursor..end) | ||
| .unwrap_or(&[]) | ||
| .iter() | ||
| .map(|s| s.to_string()) | ||
| .collect(); | ||
| Ok(Page { | ||
| lines, | ||
| next_cursor: end, | ||
| total_lines: total, | ||
| has_more: end < total, | ||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
| }) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[tokio::test] | ||
| async fn read_page_handles_binary_log_without_empty_output() { | ||
| // Simulate a command that writes non-UTF-8 bytes to its log. | ||
| let dir = tempfile::tempdir().unwrap(); | ||
| let log = dir.path().join("bin.log"); | ||
| // Valid UTF-8 prefix + invalid bytes + valid suffix. | ||
| tokio::fs::write(&log, b"line1\nline2\xff\xfeline3\n") | ||
| .await | ||
| .unwrap(); | ||
| let page = read_page(&log, 0, 100).await.unwrap(); | ||
| // Must return lines, not an empty page. | ||
| assert!( | ||
| !page.lines.is_empty(), | ||
| "binary log must not produce empty page" | ||
| ); | ||
| assert!(page.lines[0].contains("line1")); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn read_page_clamps_zero_limit_to_avoid_infinite_loop() { | ||
| // limit=0 must not produce an empty, non-advancing page: a client paging | ||
| // on next_cursor would otherwise loop forever on the same window. | ||
| let dir = tempfile::tempdir().unwrap(); | ||
| let log = dir.path().join("multi.log"); | ||
| tokio::fs::write(&log, b"a\nb\nc\n").await.unwrap(); | ||
| let page = read_page(&log, 0, 0).await.unwrap(); | ||
| assert_eq!(page.lines.len(), 1, "zero limit must clamp to one line"); | ||
| assert!(page.next_cursor > 0, "cursor must advance past the start"); | ||
| assert!(page.has_more, "more lines remain after the clamped page"); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn read_page_errors_on_missing_log() { | ||
| // A missing log is an error, not a silently-empty page. | ||
| let dir = tempfile::tempdir().unwrap(); | ||
| let missing = dir.path().join("nope.log"); | ||
| assert!(read_page(&missing, 0, 100).await.is_err()); | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.