Skip to content

Add diagnostic logs, shown through mapbox history show - #58

Merged
zmofei merged 2 commits into
run-logfrom
diagnostic-log
Sep 29, 2026
Merged

zmofei merged 2 commits into
run-logfrom
diagnostic-log

Conversation

@zmofei

@zmofei zmofei commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

Stacked on #56 (command history), which is stacked on #57.

Adds optional diagnostic logs for troubleshooting. History keeps only what is safe without anyone having asked; this keeps what answers "why did that command fail". It is off by default and reached only through mapbox history show — there is no separate logs command.

  • mapbox config set log on (or MAPBOX_LOG=1 for a session) writes, for each run history records, one line to ~/.mapbox/logs/<UTC date>.jsonl, linked to the history record by its id: the command line, which token was used (source, type, account — never the token), each request (method, URL, status, request id, timing) and the error message. Tokens are redacted in argv and URLs, and every string is also scrubbed of token-shaped words.
  • Logging needs history. With history off it never runs, and with the history setting off config set log on fails with history_required instead of storing a setting that does nothing. A run history doesn't record (--help, sudo, ...) gets no log either.
  • history show reports diagnostics.status: captured (the log is included), not_captured (logging was off for that run) or unavailable (captured, since expired or evicted). The history record carries diagnosticsCaptured so the last two can be told apart. history list marks runs with a log.
  • Kept up to 30 days and 100 MB in total. Past the limit the oldest logs go first, down to the line, and their history records stay. On every run, a day of logs whose day of history has expired or gone is deleted, logging on or off.

The three status values are new machine-readable values, documented in docs/commands.md.

Not checked: the 100 MB eviction is tested with a sparse file, not 100 MB of real lines. A run that appends while another trims can lose its line; the logs are best-effort and a lock didn't seem worth it. The history show sample in docs/commands.md was edited by hand after the review fixes, not re-captured. Nothing tests that a run's history and log land in the same day's file across midnight.

For each run command history records, `mapbox config set log on` (or
MAPBOX_LOG=1) adds a line of detail to ~/.mapbox/logs/<UTC date>.jsonl,
linked to the history record by its id: the command line with tokens
redacted, which token was used, each request (method, redacted URL,
status, request id, timing) and the error message. Off by default.

Logging needs history. With history off it never runs, and `config set
log on` refuses with history_required rather than store a setting that
does nothing. A run history doesn't record gets no log either, since
nothing could lead back to it.

`mapbox history show` includes the log and says what became of it:
diagnostics.status is captured, not_captured (logging was off) or
unavailable (captured, since expired or evicted). The history record
carries diagnosticsCaptured so the last two can be told apart. There is
no separate logs command.

Logs are kept up to 30 days and 100 MB in total. Past the limit the
oldest go first, down to the line, and their history records stay. On
every run, a day of logs whose day of history has expired or gone is
deleted, logging on or off.
- History and the log each read the clock, so a run finishing across UTC
  midnight could put its log a day after its history, and the same run's
  cleanup then deleted it. Both lines now share one time and one day's
  file, which also drops next_date.
- The log no longer repeats what the history record has (command,
  invocation, exitCode, durationMs, version).
- scrub_tokens now finds a token that starts inside a word, as after a
  percent-encoded `=` (`%3Dpk.`) or in a short-flag cluster (`-ytpk.`).
- tests/history.rs clears MAPBOX_LOG like the other suites.
- README: a day of logs goes with its day of history, and the refusal
  follows the `history` setting, not MAPBOX_HISTORY.
@zmofei
zmofei marked this pull request as ready for review September 28, 2026 12:52
@zmofei
zmofei requested a review from a team as a code owner September 28, 2026 12:52

@mattpodwysocki mattpodwysocki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the one I pushed hardest on, since it's the security-sensitive piece. Built it, ran the suite, then actually did the thing the PR body admits wasn't fully checked: passed a real token-shaped value (pk.., 40+ chars) on the command line to a request that hit a real 401, with logging on, and read the raw jsonl line back off disk.

Confirmed: --token's value is in argv, access_token= is in the request URL, and the account claim it did keep (from the token's own payload, not the token itself) is exactly what the doc says it keeps. Also confirmed history_required fires correctly when history is off and you try to turn logging on, and the not_captured status shows up correctly in history show when logging was off for that run.

Good call reusing tilesets_cli's looks_like_a_token and redacted_argv rather than writing new redaction logic for this, those are already the audited mechanism this codebase trusts for the same job in --debug output.

One read worth double-checking on your end rather than mine: scrub_tokens finds the leftmost token-shaped suffix of a token-char run and redacts from there to the end of that run. That matches every test case here, but it does mean if a URL ever legitimately has two token-shaped-looking segments in the same unbroken run of token characters, only the first one triggers redaction and the rest of that run goes with it anyway (redacted as one unit) rather than each being found independently. Given how token-shaped detection works (40+ chars starting with pk./sk./tk.) I don't see a real string that would exercise this differently, just noting I didn't construct an adversarial case beyond the ones already in the test suite.

Approving.

@zmofei
zmofei removed this pull request from stack #59 September 29, 2026 08:37
@zmofei
zmofei merged commit ffc8a97 into run-log Sep 29, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants