Add completion spec: journalctl - #299
Conversation
Adds the journalctl signature (options and dynamic arguments modelled on journalctl --help and the fish completion) plus its generators: system and user units, boot offsets, journal field names, field matches and syslog identifiers. The systemd unit listing systemctl already had moves to common.rs so both commands share it. Co-Authored-By: Warp Agent <agent@warp.dev>
|
@warp-agent-staging[bot] I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds a journalctl completion signature, dynamic generators for units/boots/fields/syslog identifiers, tests, and a shared systemd unit generator refactor reused by systemctl.
Concerns
--setup-keysgenerates FSS key material and can overwrite existing keys with--force, but the completion spec does not mark it dangerous.
Security
- The new signature should warn before suggesting
--setup-keys, because accidental use can alter journal sealing keys and affect journal verification.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR adds the journalctl signature, dynamic generators, tests, and a shared systemd-unit generator used by systemctl; the code passes the review rubric, but the required generator-evidence exception requires a human decision.
Concerns
❓ [QUESTION] The local completion-spec standard requires valid Warp dropdown screenshots for each added generator. The documented headless/no-systemd environment cannot produce valid evidence, while the ticket permits registry/unit proof for signature data; please decide whether to accept this exception or arrange systemd-host verification. The unresolved --setup-keys danger-marker finding is being relayed to implementation separately.
Verdict
Checks: CI ✅ · Rust format/lint ✅ · Rust tests ✅ · npm formatter local check unavailable (prettier is not installed; CI format ✅) · visual evidence exception pending
Found: 0 critical, 1 important, 0 suggestions
Prior concerns still outstanding: --setup-keys lacks isDangerous: true.
Request changes
Review run
https://oz.staging.warp.dev/runs/019fcf65-d87f-7eac-863d-4e1b881f1812
--setup-keys writes Forward Secure Sealing key material and --force overwrites an existing key pair, while --rotate irreversibly archives the active journal files. Mark all three alongside the --vacuum-* options and lock the set in with a test against the raw spec, since isDangerous is not carried through the CommandOption to Opt conversion. Co-Authored-By: Warp Agent <agent@warp.dev>
Requested by the task requester. src/lib.rs returns to its state on main; the generator parser tests in generators/journalctl_tests.rs are unaffected. Co-Authored-By: Warp Agent <agent@warp.dev>
## Description Updates `warp-command-signatures` from `29cd61c3` to `a2ad4bfb` so the client picks up the new `journalctl` completion spec. This is the client-side half of APP-3968; the spec itself merged in warpdotdev/command-signatures#299. ### Merged PRs - Add completion spec: journalctl (warpdotdev/command-signatures#299) - add specialized review skill (warpdotdev/command-signatures#289) — repo-internal `.agents/skills/` docs, no runtime effect - fix: update serde_with to 3.21.0 to resolve GHSA-7gcf-g7xr-8hxj (warpdotdev/command-signatures#295) — same fix the previous pin already carried, now via `main` The only functional change reaching the client is the `journalctl` spec: 86 option names plus six generators (`units`, `user_units`, `boots`, `journal_fields`, `journal_field_matches`, `syslog_identifiers`). The diff is 3 lines — the `rev` in `Cargo.toml` and the two matching `source` lines in `Cargo.lock`. The two revs have **identical dependency manifests** (verified with `git diff <old> <new> -- Cargo.toml */Cargo.toml`, empty), so no other lockfile entry legitimately changes. Note that `cargo update -p warp-command-signatures` additionally re-resolved ~9 unrelated transitive references (`windows-sys`, `windows-core`, `base64` downgrades); that churn is not attributable to this bump, so it was reverted in favour of the targeted `source` edit. `cargo metadata --locked` passes, confirming the lockfile is consistent. ## Verification - **`journalctl` now resolves through the completer path.** Exercised `warp_command_signatures::signature_by_name("journalctl")` and `dynamic_command_signature_data()` — the two calls `CommandRegistry::new_with_embedded_signatures()` makes in `crates/warp_completer/src/signatures/legacy/mod.rs` — against the new rev with `default-features = false, features = ["embed-signatures"]`, matching what `warp_completer` enables on non-wasm. Result: signature resolves ("Query the journal", 86 option names, all of `-u/--unit`, `-b/--boot`, `-p/--priority`, `-o/--output`, `-f/--follow`, `-k/--dmesg`, `--user-unit`, `--setup-keys` present) and all six generators are registered. An unknown command still resolves to `None`, confirming embedded data is genuinely being read. - **Before/after.** The same check against the *old* pin `29cd61c3` returns `None` for `journalctl`, so this bump is what delivers the signature. - **`cargo check -p warp_completer --locked`** — clean. - **`cargo test -p warp_completer --locked`** — 132 passed, 25 failed. Those 25 failures are **pre-existing and unrelated**: running the identical command on a clean `origin/master` checkout without this change produces the byte-identical `132 passed; 25 failed`. They all panic in `warp_features` with "Tried to check FeatureFlag before feature flags were initialized", which happens because a single-crate test run does not pull in `warp_features/test-util` the way a workspace run does. Not introduced here. - No formatting/clippy run: this change touches only `Cargo.toml` and `Cargo.lock`, with no Rust source, so `./script/format` and clippy have nothing to act on. The workspace-wide `./script/presubmit` was deliberately not run — disproportionate to a manifest-only pin, and CI covers it. - No GUI/computer-use proof: the requester waived visual proof on this task, and this is a dependency pin. Worth flagging for reviewers: the two spec-surface regression tests that briefly existed in command-signatures were removed at the requester's request before #299 merged, so **the client-side resolution check above is currently the only automated proof that the `journalctl` signature is reachable.** Nothing in either repo will fail if the spec is later dropped or renamed. CHANGELOG-IMPROVEMENT: Added completions for `journalctl`, including units, boots, journal fields, and syslog identifiers. Originating thread: https://warpdev.slack.com/archives/C0BDQDW8V5E/p1785889196827689 <!-- factory-agent: {"source":"factory-agent","task_id":"APP-3968","task_source":"linear","task_url":"https://linear.app/warpdotdev/issue/APP-3968/add-journalctl-command-completions","oz_run_id":"019fcf55-544f-79eb-b392-96007f11dcad","repo":"warpdotdev/warp"} --> _This PR was generated with [Oz](https://warp.dev/oz)._ Co-authored-by: Warp Agent <agent@warp.dev>
Summary
Adds a completion spec for
journalctl(systemd journal viewer), which Warp did not ship a signature for — typingjournalctl <tab>fell back to generic path completions. Resolves warpdotdev/warp#9778.command-signatures/json/journalctl.json— the full option surface, transcribed fromjournalctl --help(systemd 255) and cross-checked against the fish completion: source, filtering, output-control, pager, FSS and command options. Static enumerations come from the tool itself — priorities, the 16 output modes (journalctl --output=help) and the syslog facilities (journalctl --facility=help).--vacuum-*are markedisDangerous;--system/--userare mutually exclusive; path-taking options use thefolders/filepathstemplates.command-signatures/src/generators/journalctl.rs— six generators:units,user_units,boots,journal_fields,journal_field_matches(field names with a trailing=for the positionalFIELD=VALUEmatches) andsyslog_identifiers.command-signatures/src/generators/common.rs— the systemd unit listing previously inlined insystemctl.rsmoves here assystemd_units_generator()/systemd_user_units_generator(), per the repo's rule that generators shared by multiple commands live incommon.rs.systemctlnow calls the shared one; its command string and parsing are unchanged.Non-systemd hosts fail soft: every
journalctlgenerator runs throughsingle_command_and_ignore_stderr, and the unit generators useCommandBuilder::pipe, which already discards stderr — so a missing binary or unreadable journal yields no suggestions rather than error text. No generator follows the journal or blocks.Verification
./script/presubmitis green (npm run format:check,cargo fmt --check,cargo clippy -D warnings,cargo test— 77 passed, 0 failed).command-signatures/src/generators/journalctl_tests.rsadds 10 parser tests covering the modern and legacy--list-bootsshapes, boots without timestamps, non-boot lines, field names as option arguments vs.FIELD=matches, and unit de-duplication.This PR carries no regression test over the spec surface itself. Two such tests existed (
journalctl_signature_offers_its_primary_optionsandjournalctl_marks_destructive_options_dangerous, both verified failing-before/passing-after) and were removed at the requester's explicit request in the originating Slack thread ("Delete the tests you added in command-signatures/src/lib.rs").src/lib.rsis byte-identical tomain. The trade-off: the spec's presence, its option surface and the danger markers are no longer locked in by a test, so a future edit can silently drop them. The repo's existing generic tests still cover deserialization and generator-name resolution for every spec.The generator commands were also exercised against a real
journalctl(systemd 255) reading a synthesized journal, confirming the parsers match live output:Visual proof — waived by the requester
.agents/skills/add-command-specasks for a Warp dropdown screenshot per generator. That evidence could not be produced from this cloud agent: the sandbox is a headless container with no display and no systemd (journalctl/systemctlare absent, so every generator would render an empty menu), and a Warp build there would have nothing to list.The requester reviewed that exception and waived the screenshot requirement in the originating Slack thread ("Forget the screenshot"). The generators stand on the deterministic evidence above. Anyone wanting the dropdowns later can reproduce them on a systemd host with
.agents/skills/test-local-warp(point warp'swarp-command-signaturesdependency at this checkout,cargo run --features fast_dev, thenjournalctl -u <tab>,-b <tab>,-t <tab>,-F <tab>,journalctl <tab>,--user-unit <tab>).Rework changes
Review feedback addressed in rework #1:
--setup-keyslacksisDangerous: true— fixed, and extended to the two siblings with the same destructive character:--force(its only role injournalctlis overwriting an existing FSS key pair, which is exactly what makes--setup-keysdestructive) and--rotate(irreversibly archives the active journal files). These join the--vacuum-*options already marked.--flush(relocates journal data from/runto/var, preserving it),--sync(write-only),--relinquish-var/--smart-relinquish-var(change where future logging goes; the latter is a no-op when the log directory is on the root mount), and--verify/--update-catalog/--header(read or rebuild the catalog index). Happy to widen the set if reviewers prefer a broader bar.is_dangerousis parsed on the Fig-schema type but dropped byFrom<CommandOption> for Opt(completion-metadata/src/fig_types.rs:485), so an option-levelisDangeroushas no runtime effect on completions today — here or in the other 47 specs that set it. The marker is still correct and consistent as spec data, but it will not surface a warning in the client until that conversion carries the flag. Worth a separate issue; out of scope here.src/lib.rsregression tests added earlier in this PR were removed at the requester's request — see the Verification section above.Follow-up
warpdotdev/warpneeds awarp-command-signaturesgitrevbump after this merges for the client to pick the signature up. No warp PR is opened here.Originating thread: https://warpdev.slack.com/archives/C0BDQDW8V5E/p1785889196827689
This PR was generated with Oz.