Skip to content

Follow-ups from #38: doctor permission check, docs gap, and token-resolution cleanup #40

Description

@DevRohit06

Items surfaced while reviewing #38 that were deliberately left out of that PR's scope. Roughly in priority order.

1. Existing loose config files are never tightened

#37 asked to "automatically tighten permissions if existing files or directories had looser permissions," but tightening only happens inside save_config(). A user who already has ~/.discli/config.json at 0644 and never runs config set again stays exposed indefinitely — which is most of the population the issue was about.

doctor is the documented first-stop diagnostic and was already touched by #38, so a check that stats the config file and reports a loose mode is the natural fix. Tightening silently on read is the other option, but a chmod as a side effect of load_config() is surprising; the doctor check is the better shape.

2. Dangling code block in docs/getting-started/configuration.mdx

The edit removed the lead-in sentence ("This writes to ~/.discli/config.json:") along with the Set token. output, leaving a bare json block after the bash command with nothing introducing it.

3. CLAUDE.md not updated for DISCORD_TOKEN

  • Line 39 lists only DISCORD_BOT_TOKEN under Environment Variables.
  • Line 44's note — "examples/meeting_transcriber.py reads DISCORD_TOKEN, not DISCORD_BOT_TOKEN" — changes meaning now that DISCORD_TOKEN is a supported fallback rather than a mismatch.

The four-level resolution order is also worth a line there, since it is the kind of non-obvious ordering decision CLAUDE.md exists to record.

4. test_setup_reuses_discord_token_when_confirmed doesn't test its name

It asserts exit_code == 0 and that "1. Bot token" printed — neither of which depends on DISCORD_TOKEN being set at all. The wizard fixture already captures saves, so assert wizard["token"] == "discord-token" is available.

Related, and worth a deliberate decision rather than a drive-by: confirming that candidate copies the env token into config.json. That is pre-existing behaviour for DISCORD_BOT_TOKEN, but the docs pitch env vars as the way to "avoid writing secrets to disk" and the wizard quietly does the opposite. #38 doubled the surface where that happens.

5. Token resolution is implemented twice

cli.py:78-79 and client.py:28-30 each implement the config → DISCORD_TOKEN fallback. Every production call site is resolve_token(ctx.obj.get("token"), {}) — an empty dict — so client.py's config branch was already dead in-tree and its env branch is reachable only from tests. Two places to keep in sync for one rule, with only one of them live.

6. Smaller cleanups

  • doctor.py: the token = os.environ.get(...) or load_config().get(...) or os.environ.get(...) line is duplicated verbatim at two sites and runs 108 chars. A small shared helper keeps them from drifting apart.
  • config.py uses sys.stderr.write + explicit flush where the rest of the codebase uses click.echo(..., err=True).
  • save_config() is still non-atomic (pre-existing, not a regression from feat(security): enforce 0600 config permissions and support DISCORD_TOKEN fallback #38). os.open a temp in the same directory at 0600 → write → os.replace would get atomicity, never expose content, and remove the fchmod dance entirely. Optional, but the function was already being rewritten once.

Not in scope here

The fd double-close and the silent unprotected-write fallback from #38 were fixed in e2057f8 before merge.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions