Repository navigation
v1.22.1 - #51
Merged
Merged
v1.22.1#51
Conversation
Comments here are load-bearing — security rationale, protocol quirks,
error strings worth matching on — so a naive volume cut would delete
the value. The problem was never density (10.2% overall, normal for
Go) but altitude: restatements of the next line, narration of past
changes, and references to a planning process that does not exist in
the repo.
The policy in AGENTS.md is deliberately worded as "required, not
optional" for the categories that matter. A plain "explain why, never
what" reads against the standing instruction to add no comments, and
the next person to touch this tree would take that as licence to strip
the security rationales out.
Applied to production code only; test comments document real protocol
behaviour and stay. Every comment touching validation, TLS, key
handling, timeouts or the wire format keeps its reason.
Changes, by kind:
- Dropped the two "Phase 2" planning references (sms/send.go,
ipc/server.go). The ipc one gained the actual reason for the
chmod, which is worth more than the label was.
- Converted change narration to its conclusion in 8 places: "used to
fire on any message containing fusermount" now states the narrow rule
directly, "a graceful stop used to leave every mount live" now says
it must unmount explicitly.
- Removed 20 inline restatements ("Send our local battery to the
phone", "Read capacity (0-100)", "Send OK response to indicate
stream is starting").
- Merged internal/log/log.go's four identical level comments into one
statement of why the forwards exist.
- Split two comments that had fused a contract to a history:
mounttable.go's liveMountPoint had run straight into mountExists'
doc without a blank line, and sftp.Info repeated "returns nil" twice.
- Compressed sftp.UnmountAll from 23 comment lines to 11 by moving the
ctx contract into the godoc where it belongs and the shutdown
rationale into the body. The ctx semantics are load-bearing — they
are the fix for a race where the caller returned while goroutines
were still logging — so they stay verbatim in substance.
- Added the reason for AllowedTimestampDiff (clock skew bound) and
explained what ShareBody carries that Android's packet type omits.
Enforcement: enabled godot and godox. Both are already available —
golangci-lint is pinned and already runs in hk and CI — so this adds
no dependency and no workflow change. godox reports zero today and is
pure insurance; godot caught the two comments above, which is how they
were found rather than by eye.
Comment lines, production Go: 2449 -> 2422. The small number is the
point: this is a precision pass, not a diet. The density was never the
problem.
Issue #50: `kcd mpris seek -10s` was rejected as an unknown flag. The offset parser was always right about "-10s"; urfave/cli never handed it the string, because it reads any argument starting with "-" as a flag. `-- -10s` worked around it, so the help text was advertising a form that did not parse. `kcd mpris seek` now sets SkipFlagParsing and recovers --device/--player itself. A "-" followed by a digit is the offset, which is unambiguous here because neither flag takes a numeric value. Anything else starting with "-" stays an error, so a typo like "--vol 5" cannot seek the wrong device by being silently ignored. SkipFlagParsing also stops urfave/cli handling -h, so that is recognised and routed to the subcommand help. Second fix, same plugin. A relative seek is sent to the phone as an absolute SetPosition, since phones implement SetPosition but ignore Seek. That conversion needs the tracked playback position, and when there was none the old code fell through and sent nothing at all -- a no-op that still reported success, which is the failure mode #49 set out to fix. It now returns an error naming the cause. An absolute setPosition or bare number needs no cached state and is unaffected. While here: SendAction's doc comment had drifted onto the canonicalActions var two declarations above it, so the function that sends on two packet types documented none of it. Verified against a paired phone with a track playing: -10s, -20s, +30s, +20s and the absolute 1m30s all move the position, `-- -15s` still works, and clamping holds at both ends of the track.
The `setVolume` snippet in CLIENT_GUIDE was wrong in a way that would
have wasted someone's afternoon: MprisActionPayload has no `value` key,
so {"action": "setVolume", "value": 50} unmarshals to a nil Volume,
sends a bare action, and returns ok. Volume has always been its own
field. Both copies corrected, and seek added alongside since the
relative/absolute distinction is the thing a client author has to get
right.
Also:
- CLI.md listed `kcd sftp mount` twice, once without the [--ro] flags
and once with, and never said where mounts land by default. Merged
into one synopsis.
- IPC_PROTOCOL.md did not say that a relative seek without a tracked
position is an error. A client author reading it would assume a
no-op.
- ARCHITECTURE.md's mpris row predated both the action-name canonical
mapping and the SetPosition conversion.
- device_id was the only scalar config key absent from the example
TOML. It is generated and written back automatically, and editing it
breaks pairing for every device, so it is worth a warning next to the
other identity keys.
Checked while here, all correct: all 32 event types appear in
IPC_PROTOCOL.md; every CLI subcommand and flag is documented; the other
84 scalar config keys are present in the example TOML.
SMS content reached a client three ways, only one intentional. A bare `kcd watch` subscribed to every event, so it both received message bodies and armed the phone, since arming was gated on having a subscriber for sms.incoming. `kcd sms conversation` and `conversations` are on-demand history reads, but they latch the phone armed. That latch is the root cause and it is not ours to clear: Android sets haveMessagesBeenRequested from any sms.request* packet (SMSPlugin.kt:313,327) and nothing resets it, not even onDestroy. Once armed, the phone pushes every new message for the life of the connection. So `always_arm = false` only ever stopped kcd from asking; it could not stop kcd from being asked by something else, and could never un-ask. Two changes, because one gate is not enough against a one-way latch: - An empty watch filter no longer means "everything". It means every type except the opt-in ones, so SMS has to be named with -e. This closes both halves of the bare-watch problem: no body delivered, and no arming triggered. - The bus publish is gated on [sms] publish_incoming, default false. This is the half that matters, because it holds even when the phone was armed by something else. notify_incoming is untouched and independent, so a desktop popup does not imply the body is exposed over IPC. sms.attachment stays published but is excluded from a bare watch. Its payload is a saved path with no message body, and it is the only way the `kcd sms attachment` command can report where the file landed. All() is new, built from the event constants and checked against bus.go by parsing it — a new event added without being listed would otherwise be silently dropped from every default watch stream. The packets still cross the wire once the phone is armed; this stops content leaving the daemon, not the phone sending. Recorded under Known Gaps in AGENTS.md, along with the fact that ending it means disabling the plugin on the device.
The config flag was redundant and inert where it mattered. armed() never consulted it, so setting it did not stop the phone being armed — it only suppressed the publish. The result was that naming `-e sms.incoming` armed the phone and then delivered nothing, silently, with no error and no hint of what to set. The filter is sufficient on its own, and is the only real gate. Once an unfiltered subscriber stops matching SMS, "has a subscriber for sms.incoming" means "someone typed -e sms.incoming". Every subscriber in the daemon names its types explicitly, so there is no unfiltered one left to defend against. Enforcement also moves into Subscriber.matches rather than staying in handleWatch. It was in the watch handler last time, which meant any future internal caller writing bus.Subscribe(0) would silently re-arm the phone. One unfiltered path existed this session — my own test — which is how this showed up. matches() is the chokepoint both delivery and HasSubscribers go through, so it is the only place the rule can be bypassed deliberately rather than by oversight. handleWatch now passes its filters through unchanged. So the opt-in is one word: kcd watch # no SMS, no arming kcd watch -e sms.incoming # arms and delivers Checked a backlog question while here: an armed phone with no listener does not queue. sendLatestMessage advances mostRecentTimestamp on every push, subscribed or not, so someone who subscribes later sees only genuinely new messages. Tests: TestNamingSMSDeliversAndArms covers the consent path in one go — an unfiltered subscriber receives nothing and leaves HasSubscribers false, then naming the type arms the phone and delivers the body. The four flag tests are gone, since the flag is gone.
The gap: the only non-interactive pairing mode was -y, which trusts the
first device that asks on the local network. --expected-fingerprint and
--known-only both require knowing the peer in advance, which is exactly
what a first-time pairing does not give you. A client without a prompt
was therefore pushed toward trusting the network by the headless framing
-- right for a server, wrong for a panel.
Every primitive needed already existed, so this is CLI plumbing only. No
daemon, IPC or protocol change: broadcast_start maps to StartOwned(ctx,
OwnerPairing) and broadcast_stop releases it.
kcd pair --advertise-only # advertise, accept nothing, Ctrl+C
kcd pair --advertise-only --json # {"advertising":true|false} on stdout
The client then keeps its existing watch stream, reads candidates from
pair.requested (which already carries verificationKey), shows the code to
the owner, and accepts per device with `kcd pair <device-id>`. Acceptance
stays a separate, explicit, per-device step -- --advertise-only never
calls CmdPair, asserted directly by the test rather than inferred.
--json prints both transitions so a supervising client knows when the
daemon is genuinely discoverable, not merely that a process started.
Broadcast stops on every exit path including SIGTERM, and stopping also
drops the discovery connections that never led to pairing, so strangers
do not linger.
Contradictory flags are refused rather than silently dropped: combining
with --yes or a device ID is an error, since the caller is a program
trusting "never accepts".
advertiseUntil takes the stop condition as a parameter rather than
handling signals itself, so the tests drive it with a channel instead of
signalling the test binary -- my first attempt did the latter, which is
both fragile and rude to the rest of the suite.
Verified live: both modes emit and terminate correctly, and listen mode,
-y, --known-only and --expected-fingerprint are unchanged.
`kcd battery --json` with no positional printed `"deviceId": ""` while correctly querying the auto-selected device. The action resolved the ID and then reported the raw positional instead of the resolution. One-token root cause, but the regression was invisible: charge and charging were right, only the identifying field was wrong, and only on the convenience path. runBattery is now a separate function taking the client, so the test can wire a stub IPC server and assert the reported ID equals the auto-resolved one. Also documented in CLI.md that flags must precede positionals. Trailing flags are silently ignored under urfave/cli v2's POSIX parsing, which is what made `kcd battery <id> --json` print text instead of JSON during verification. No code change there: upstream v2 offers no trailing-flag mode, and per-command hacks would be worse than the convention.
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.
SMS is now opt-in
A bare
kcd watchprinted incoming message bodies (sms.incomingwith sender and text). Message delivery is now consent-gated atSubscriber.matches, the bus chokepoint no caller can bypass: unfiltered subscribers never match the SMS types, naming them in--eventsis the opt-in. .sms.attachmentstays published but excluded from bare watch: no message body, and it is the only waykcd sms attachmentreports the saved path.Android's latch (
haveMessagesBeenRequested) is one-way and not fixable from kcd; recorded under Known Gaps in AGENTS.md. Mitigated, not solved.Pairing for clients that cannot prompt
kcd pair --advertise-only [--json]: opens the advertisement window and accepts nothing. Panels and widgets keep their existing watch stream, read candidates frompair.requested(already carriesverificationKey), and accept per device withkcd pair <device-id>. Combining with--yesor a device ID is an error. CLI plumbing only — no daemon, IPC, or protocol change. Documented in CLI.md, CLIENT_GUIDE.md (do not shell out tokcd pair -y), and IPC_PROTOCOL.md.Fixes
battery --jsonwith no positional reported"deviceId": ""while querying the right device. Now reports the resolved ID;runBatteryextracted for testability.mpris seekrejected negative values (flag parsing) and silently did nothing when no position was tracked.SkipFlagParsing+splitSeekArgs; relative seeks without a tracked position now error.Docs and hygiene
godot/godox; narration stripped, conclusions kept.setVolumeexample (novaluekey), a duplicatedsftp mountsynopsis, and three smaller drifts.All gates green locally:
go vet,golangci-lint0 issues,go test -race ./..., integration, static build,hk check --all,goreleaser check. Live-verified against a connected phone: negative seek, battery JSON both paths, advertise-only start/stop in both modes.