feat: add a read-only submit --dry-run - #47
Conversation
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Droid encountered an error —— View job |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestions only — one consistency nit inline.
Reviewed changes
Read-only submit --dry-run across lighthouse_cli/cli.py, lighthouse_cli/submit.py, and tests/test_submit.py (1 commit). Verified locally: tests/test_submit.py passes 95/95, including the 5 new dry-run tests.
--dry-runflag wiring —cli.pyadds the flag and threads it intocmd_submit, which resolves the course and folder as usual, then reports the destination and exits before the confirmation prompt, the file-body read, and the upload.- Read-only client, no
--yesgate — dry-run buildsLighthouseClient(read_only_auth=True)(no cookie migration/CDP refresh; same pattern asquiz_preview_session.pyandauth.py) and bypasses the non-interactive--yesrefusal. The real submit path is untouched and still gated. _submit_dry_runoutput — stats the file for size only, reportsfolder_verified: falsewith a fixed warning when the folder-detail lookup falls back to the default name, and keeps the--jsonstdout-only contract (human text only without--json; errors via_submit_errorto stderr). No secrets reach output — names go throughsafe_display_textand the warning is a fixed string.- Tests —
TestSubmitDryRunpins the right invariants:Path.read_bytespatched to raise (no body read),client_cls.assert_called_once_with(read_only_auth=True),submit_filenever called, the--yesbypass, the unverified-folder warning, resolution-error JSON on stdout, and the unchanged real-submit gate. Assertions are exact (full-payload equality), so they fail if the behavior drifts.
openai-compatible/glm-5.3 ｜ 𝕏
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Fix these issues in Kilo Cloud Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
Previous Review Summaries (2 snapshots, latest commit 44a9602)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 44a9602)Status: 1 Issue Found | Recommendation: Address before merge Fix these issues in Kilo Cloud Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Previous review (commit 1471802)Status: 2 Issues Found | Recommendation: Address before merge Fix these issues in Kilo Cloud Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Reviewed by glm-5.3-flash · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
'lighthouse submit COURSE FOLDER --file F --dry-run [--json]' resolves the course and dropbox folder with a read-only client and prints the destination (course, folder, file name and size). It never reads the file body or uploads, needs no --yes, and flags a folder name it could not verify. Ported from the MAHE-useful part of #28 without its trial --site option. Refs #28 #25
From pullfrog and Kilo reviews of 1471802: - folder_verified now means the folder's details were read, not that its name differs from the display fallback, so a folder literally named "Unknown folder" is no longer flagged. - Build the client as LighthouseClient(read_only_auth=dry_run), like download --dry-run. - submit --help documents the dry-run JSON shape.
1471802 to
44a9602
Compare
There was a problem hiding this comment.
ℹ️ No critical issues — one documentation gap inline.
Reviewed changes
Delta since the prior pullfrog review (44a9602): verification is now reported explicitly instead of being inferred from the fallback folder name, and the run-1 consistency nit is applied. Verified locally: tests/test_submit.py 96/96, plus test_cli_contracts.py + test_read_only_auth.py 146/146.
- Applied
read_only_auth=dry_run— the client is now constructed with the flag passed straight through (tests pinread_only_auth=Falsefor real submits), matching thedownload --dry-runprecedent and closing the run-1 thread. - Moved verification into
_get_folder_name— it returns(name, verified)based on whether the folder-detail lookup actually succeeded, replacing thefolder_name != _DEFAULT_FOLDER_NAMEheuristic; a folder literally named "Unknown folder" is no longer falsely reported unverified (test-pinned in both directions), and the warning was reworded to "The folder details could not be read…". - Documented the dry-run output — the
submithelp and docstring now list the dry-run JSON fields (dry_run,folder_verified, …) and the warning condition.
No secrets reach output (names go through safe_display_text, the warning is a fixed string), the --json stdout-only contract holds on every new path, and resolution errors still emit the usual single JSON error document.
openai-compatible/glm-5.3 ｜ 𝕏
|
Droid encountered an error —— View job |
From Kilo's review of 44a9602: a readable folder whose Name is missing, empty or rejected for display came back as "Unknown folder" with folder_verified true and no warning. folder_verified now also requires a usable name.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Delta since the prior pullfrog review (44a9602) is documentation only, and it closes that review's inline thread: the README submit section now covers --dry-run. The full diff was re-read for context; no code or tests changed in this delta.
- Documented
--dry-runin the README — usage line, a flags-table row (read-only resolve, no file-body read, no upload, no--yesneeded), a dedicated dry-run JSON example, and thefolder_verified: false+warningnote; each documented field and claim was checked against_submit_dry_run's payload (lighthouse_cli/submit.py:182-193) and matches, including the warning semantics (added only when the folder-detail lookup failed, exit code still 0).
openai-compatible/glm-5.3 ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Delta since the prior pullfrog review (f18d089): one commit tightening when a dry-run reports folder_verified: true. Verified locally: tests/test_submit.py passes 99/99 (dry-run selection 8/8).
- Required a usable folder name for verification —
_get_folder_name(lighthouse_cli/submit.py:353-362) now returnsverified=Falsewhen the detail lookup succeeds but the projected name is empty (missing/blank, over the 256-char display cap, non-printable, or redacted bysafe_display_text), closing the empty-Nameedge the prior run had noted; a folder literally named "Unknown folder" still verifies. The real-submit path is unaffected — its display name resolves identically to before, and only dry-run reads the flag. - Kept wording in sync — the warning string, README note, and docstrings now say "name" instead of "details", matching the tightened semantics.
- Strengthened the test — the unusable-detail case is now parametrized over a lookup failure,
{"Name": ""},{}, and an overlong name, asserting the exact fallback name and warning text, so it can actually fail on drift.
openai-compatible/glm-5.3 ｜ 𝕏
| _DEFAULT_FILE_NAME = "Unknown file" | ||
| _CLIENT_INIT_ERROR = "Could not initialize Lighthouse client." | ||
| _DRY_RUN_UNVERIFIED_WARNING = ( | ||
| "The folder name could not be read; check the folder ID. No submission was sent." |
There was a problem hiding this comment.
SUGGESTION: Warning says the folder name "could not be read", but the unverified path now also fires when the name was read yet unusable for display
Since 40b827f, _get_folder_name returns (_DEFAULT_FOLDER_NAME, False) whenever safe_display_text rejects Name — including a successfully read name that only fails the display projection: longer than _MAX_DISPLAY_NAME_LENGTH (256), whitespace-only, or caught by the secret/object heuristics. The new {"Name": "x" * 300} test case pins this. For such input the emitted warning "The folder name could not be read; check the folder ID" is misleading: the name was read, only its display projection was rejected, and folder resolution itself succeeded, so "check the folder ID" sends the user down the wrong path. Consider wording that covers both cases. The same "could not be read" phrasing also appears in the README note (README.md:969-970) and the submit docstring (cli.py:534), and the parametrized test name asserts "could not be read" for the over-long case.
| "The folder name could not be read; check the folder ID. No submission was sent." | |
| "The folder name could not be confirmed; check the folder ID. No submission was sent." |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
Summary
Adds a read-only
submit --dry-run, the part of #28 that is still useful for Lighthouse:--yes, even when run non-interactively.folder_verified: falsewith a warning when the folder name couldn't be confirmed.--yeswhen non-interactive and builds its client as before.#28's
--siteoption and trial session messages are intentionally not ported; #46 removes trial support.Validation
check_secrets.pyall pass; pytest reports 1580 passed.--yesneeded, an unverified folder, a resolution error, and a real submit still requiring--yes.Refs #28 #25