Skip to content

chore: make the CLI Lighthouse-only; drop the trial connection - #46

Merged
rabesss merged 2 commits into
mainfrom
chore/mahe-only
Oct 1, 2026
Merged

rabesss merged 2 commits into
mainfrom
chore/mahe-only

Conversation

@rabesss

@rabesss rabesss commented Sep 24, 2026 •

Copy link
Copy Markdown
Owner

Summary

Makes the CLI Lighthouse-only: MAHE Manipal's Brightspace. The D2L trial was a test sandbox, so its code leaves the product. Diff: +246 / −484.

  • Trial code removed: the trial connection and every --site option (the instructor/student groups and auth import-session), plus the trial-only gate on instructor previews.
  • Instructor previews are now a Lighthouse instructor feature. An account without preview rights is refused before any write: the attempt listing or the missing Start control stops it.
  • Test hook: connection.active_connection() returns Lighthouse. A private _override lets an out-of-repository harness point tests at a sandbox. The override must use its own cookie directory, and such clients never refresh or migrate authentication.
  • JSON change: the always-"lighthouse" "site" field is dropped from assessment envelopes, preview results and import-session output.
  • Storage: old trial cookies and checkpoints under sites/ are ignored and never migrated. Origin binding still rejects foreign-origin artifacts, both ways; there are tests for each direction.
  • Docs: the trial evidence log docs/assessment-coverage.md is replaced by the generic docs/quiz-protocol.md, and README examples use Lighthouse only.

Validation

  • Local gate on 8fdca9c: ruff, mypy, lint-imports, deptry, xenon and check_secrets.py all pass; pytest reports 1588 passed.
  • The secrets baseline only moved line numbers; all 29 entries remain audited.
  • The out-of-repo harness drove the unchanged instructor preview flow on the sandbox with this code: attempt 29232 went start → answer → next → answer → submit, scoring 2/2 with the receipt verified.
  • The harness's isolation self-check confirmed that requests to Lighthouse and Microsoft are refused.

Follow-ups

Review evidence (head 6b3a1d8, fixes in 8fdca9c)

  • GPT-6-Astra — medium reasoning: SHIP
  • Grok 4.7 — xhigh reasoning: SHIP
  • Z.AI GLM-5.3 — max reasoning: SHIP
  • Muse Spark 1.3 Contributor — xhigh reasoning: SHIP
  • MiMo V2.6 Flash — default variant: SHIP WITH FOLLOW-UPS. Its one P2 (a stale README sentence) is fixed.
  • CodeRabbit: not run; the free CLI allowance was exhausted.

Kilo's two suggestions are fixed, each pinned by a test that fails without the fix:

  • the override now accepts only a plain https://host origin;
  • only the built-in Lighthouse object may refresh auth.

The CLI is for Lighthouse (MAHE Manipal). The Brightspace trial was only a
test sandbox, so its code leaves the product:
- Remove the 'trial' connection, every --site option (instructor/student
  groups, auth import-session) and the trial-only gate on instructor
  previews. Instructor previews become a Lighthouse instructor feature; an
  account without preview rights is refused before any write (the attempt
  listing or the missing Start control stops it).
- connection.active_connection() returns Lighthouse. A private _override
  lets an out-of-repository harness point tests at a sandbox; it must use
  its own cookie directory, and such clients never refresh or migrate
  authentication.
- JSON output drops the always-'lighthouse' "site" field
  (assessment envelopes, preview results, import-session).
- Old trial cookies and checkpoints under sites/ are ignored, never
  migrated; origin binding still rejects foreign-origin artifacts.
- Replace the trial evidence log docs/assessment-coverage.md with the
  generic docs/quiz-protocol.md; README examples use Lighthouse only.
- Tests use the Lighthouse origin or a synthetic sandbox.example override.

Refs #25 #31

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your organization has used all 50 credits included in the free plan this billing period. To keep receiving reviews, upgrade your plan.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-24T11:42:29.174115Z 6b3a1d8 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@factory-droid

factory-droid Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Droid encountered an error —— View job

Droid Exec exited with code 1:
Tool "Edit" skipped (GPT-5.2 uses ApplyPatch instead).
Tool "Create" skipped (GPT-5.2 uses ApplyPatch instead).

Security Review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

  • Trial connection removed — connection.py replaces connection_for(site)/SUPPORTED_SITES with a LIGHTHOUSE constant and active_connection(); every --site option (instructor/student groups, auth import-session) is gone, and a repo-wide sweep finds no leftover connection_for, hetrynow, trial, or --site references in code, tests, docs, or CI config.
  • Private test-harness seam — the module-global _override is fail-closed in active_connection() (HTTPS origin + its own cookie_dir required), which structurally guarantees an override can never equal LIGHTHOUSE, so LighthouseClient always forces _read_only_auth for it; the CDP-refresh/cookie-persistence guard at lighthouse_cli/api.py:491 confirms such clients never refresh or migrate auth.
  • Preview becomes an instructor feature on Lighthouse — the trial-only PreviewWorkflow gate is removed; accounts without preview rights are refused before any write (attempt-listing failure aborts with "nothing was started", missing Start control raises PreviewRefusedError), verified against the code path.
  • Breaking --json change — the always-"lighthouse" "site" field is dropped from assessment envelopes, preview results, and import-session output; intentional and documented in the PR body and README. Worth a release note if any external script consumes this output.
  • Storage and docs — old trial cookies/checkpoints under sites/ are ignored rather than migrated (left in place, sealed); docs/assessment-coverage.md is deleted and replaced by the generic docs/quiz-protocol.md, which correctly points to the old evidence log's git history.
  • Tests — the new sandbox fixture drives the _override seam; new/changed tests genuinely pin the new behavior (override validation, origin binding in both directions, the dropped --site CLI surface, checkpoint origin binding) rather than asserting tautologies.

Full local gate on this checkout passes: ruff, mypy, lint-imports, deptry, xenon, check_secrets.py, and pytest (1582 passed) — matching the validation section.

Pullfrog  | View workflow run | Using openai-compatible/glm-5.3 | 𝕏

Comment thread lighthouse_cli/connection.py Outdated
Comment thread lighthouse_cli/api.py Outdated
@kilo-code-bot

kilo-code-bot Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 1
Issue Details (click to expand)

SUGGESTION

File Line Issue
lighthouse_cli/connection.py 35 parts.port raises ValueError (urllib's raw message) for malformed ports like :abc or :99999 instead of failing closed; the clause is redundant with the final origin == f"https://{hostname}" equality, which already rejects any ported origin
Previous findings — resolved in `8fdca9c`
  • lighthouse_cli/connection.py — origin validation now parses with urlsplit and accepts only a plain https://host origin; parametrized rejection tests cover empty host, path, userinfo, port and non-canonical case. Verified fixed.
  • lighthouse_cli/api.py — the read-only gate is now self.connection is not LIGHTHOUSE (identity); a test pins it with a value-identical copy of the Lighthouse connection. Verified fixed.
Files Reviewed (5 files)
  • .secrets.baseline - line-number move verified (132 → 150 tracks the 18 added test lines); all audited entries retained
  • README.md - "Both default to Lighthouse." removed; sentence remains accurate
  • lighthouse_cli/api.py - identity gate is not LIGHTHOUSE verified; previous suggestion resolved
  • lighthouse_cli/connection.py - 1 issue; previous suggestion resolved
  • tests/test_assessment_workflows.py - new parametrized rejections and identity-gate test verified sound

Fix these issues in Kilo Cloud

Previous Review Summary (commit 6b3a1d8)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 6b3a1d8)

Status: 2 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 2
Issue Details (click to expand)

SUGGESTION

File Line Issue
lighthouse_cli/connection.py 37 Override validation uses startswith("https://") instead of parsing the origin, so malformed origins (empty host, embedded path, userinfo) pass validation and only fail later at request/artifact time; urlsplit-based checks would fail closed
lighthouse_cli/api.py 414 Read-only gate relies on dataclass value equality (!= LIGHTHOUSE), which is only safe while active_connection() guarantees every override has its own cookie_dir; identity comparison (is not LIGHTHOUSE) expresses the invariant directly
Files Reviewed (18 files)
  • .secrets.baseline - line-number moves only, all audited entries retained
  • README.md - trial/--site examples replaced with Lighthouse-only syntax; consistent with CLI
  • docs/assessment-coverage.md - deleted (evidence log preserved in git history)
  • docs/quiz-protocol.md - new generic protocol notes; no secrets, links updated
  • lighthouse_cli/api.py - 1 issue
  • lighthouse_cli/assessment_api.py - comment-only change
  • lighthouse_cli/assessment_commands.py - --site/_site() removed cleanly; JSON envelopes updated; stderr prompt contract preserved
  • lighthouse_cli/cli.py - --site options removed; lazy import-session wiring updated
  • lighthouse_cli/connection.py - 1 issue
  • lighthouse_cli/quiz_preview_commands.py - preview_site context plumbing removed with no stale readers
  • lighthouse_cli/quiz_preview_session.py - origin-bound checkpoint validation (_load) rejects foreign-origin cursors
  • lighthouse_cli/quiz_preview_transport.py - comment-only changes
  • lighthouse_cli/session_import.py - exact-match origin binding against active_connection(); generic error keeps secrets out of output
  • tests/test_assessment_workflows.py - sandbox override fixture with own cookie dir; both directions of origin rejection covered
  • tests/test_quiz_attempt_page.py - read_only_auth=True preserves the never-refresh guarantee previously provided by site="trial"
  • tests/test_quiz_preview_finish.py - same pattern
  • tests/test_quiz_preview_session.py - same pattern plus new origin-binding checkpoint test
  • tests/test_request_protection.py - same pattern
Notes
  • The private _override seam is consumed only via active_connection() (api.py, session_import.py, quiz_preview_session.py); no production code reads LIGHTHOUSE directly, so the read-only guarantee for override clients holds.
  • Tests using the default CredentialStore() remain hermetic via the autouse LIGHTHOUSE_CONFIG_DIR fixture in tests/conftest.py.
  • No secrets introduced; JSON-on-stdout contract and stderr prompt contract are preserved on all changed command paths.

Fix these issues in Kilo Cloud


Reviewed by glm-5.3-flash · Input: 0 · Output: 0 · Cached: 0

Review guidance: REVIEW.md from base branch main

From Kilo and MiMo reviews of 6b3a1d8:
- Accept only a plain https://host override origin (no credentials,
  port, path, query, fragment or non-canonical case).
- Grant auth refresh/migration only to the built-in Lighthouse
  connection object; any override, even one equal in value to
  Lighthouse, is read-only.
- README: drop the leftover 'Both default to Lighthouse.'
@factory-droid

factory-droid Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Droid encountered an error —— View job

Droid Exec exited with code 1:
Tool "Edit" skipped (GPT-5.2 uses ApplyPatch instead).
Tool "Create" skipped (GPT-5.2 uses ApplyPatch instead).

Security Review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

Since the prior review of 6b3a1d8, one commit (8fdca9c) tightens the private connection override seam, responding to the Kilo/MiMo review feedback.

  • Tightened override origin validation — connection.py adds _is_plain_https_origin(), accepting exactly https://host: credentials, ports, paths, queries, fragments, and non-lowercase hosts are all rejected, and six new parametrized cases pin each rejection path.
  • Identity-based auth-refresh rights — LighthouseClient now grants refresh/migration only to the built-in LIGHTHOUSE object (is not rather than dataclass value equality), so even a value-identical connection copy stays read-only. test_only_the_built_in_lighthouse_object_may_refresh_auth covers both a monkeypatched value-equal copy and a validating override naming Lighthouse's origin, and genuinely fails if the check reverts to !=. Verified this is the only connection-equality privilege decision in the codebase; the other active_connection() consumers (quiz_preview_session.py, session_import.py) only read .origin/.cookie_dir.
  • Housekeeping — README drops the stale "Both default to Lighthouse." sentence and updates the JSON envelope example; .secrets.baseline shifts line numbers only, with no entry changes.

Full local gate on this checkout: ruff, mypy, pytest (1588 passed, +6 new cases), and scripts/check_secrets.py all green.

Pullfrog | View workflow run | Using openai-compatible/glm-5.3 | 𝕏

Pullfrog  | View workflow run | Using openai-compatible/glm-5.3 | 𝕏

def _is_plain_https_origin(origin: str) -> bool:
"""``https://host`` exactly: no credentials, port, path, query or fragment."""
parts = urlsplit(origin)
return (parts.scheme == "https" and bool(parts.hostname) and parts.port is None

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SUGGESTION: parts.port can raise on a malformed port instead of failing closed

For an override origin like https://sandbox.example:abc or https://sandbox.example:99999, evaluating parts.port raises ValueError from urllib.parse ("Port could not be cast to integer value as 'abc'" / "Port out of range 0-65535") instead of returning False, so active_connection() propagates urllib's raw message rather than its curated one. The clause is also redundant: the final origin == f"https://{parts.hostname}" equality already rejects any origin containing a port, so dropping parts.port is None removes the raise path entirely while keeping the validation fail-closed. Low impact - _override is a trusted test-harness seam and the exception type is unchanged - but a port typo in a harness config would surface an unfamiliar error.

Suggested change
return (parts.scheme == "https" and bool(parts.hostname) and parts.port is None
return (parts.scheme == "https" and bool(parts.hostname)

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

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.

1 participant