Address challenge feedback from Discord - #267
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f58d19972d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
|
|
||
| def check_disassembly(disas): | ||
| assert any(insn.mnemonic == "div" for insn in disas), ( |
There was a problem hiding this comment.
Require
div to be executed, not just present
Because load_shared_input() disassembles from the exported symbol through the rest of .text, this presence check can be satisfied by placing an unreachable div after ret. For this level's 10-99 inputs, a repeated-subtraction implementation can return the correct digit sum and get the flag without ever executing division, so the new level does not enforce the division concept it is meant to teach; constrain the check to the reachable instructions before ret or validate the actual quotient/remainder flow.
AGENTS.md reference: AGENTS.md:L49-L54
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
THIS MESSAGE IS GENERATED BY AN AUTOMATED PROCESS.
Addressed this feedback and pushed commit 6fd6289e.
Original feedback: #267 (comment)
Automated run notes:
- Address challenge feedback from Discord #267 (review)
- This is the review summary containing the actionable inline finding below. No separate repository change was needed for the summary itself.
- Address challenge feedback from Discord #267 (comment)
- Changed
divide-remainder's disassembly check to acceptdivonly when it appears beforesolve's firstret. A correct repeated-subtraction implementation can therefore no longer satisfy the instruction requirement by placing an unreachabledivafter returning.
- Changed
Validation:
python3 -m py_compile challenges/computing-101/numbers-as-strings/divide-remainder/challenge/.py/chal.pygit diff --checknix develop --command pwnshop test challenges/computing-101/numbers-as-strings/divide-remainder(passed: 1 challenge, 1 testcase)- In
nix develop, built and submitted a repeated-subtractionsolvewithdivafterret; the checker rejected it with the new diagnostic and did not print a flag.
|
THIS MESSAGE IS GENERATED BY AN AUTOMATED PROCESS. In response to #267 (review): Addressed this feedback and pushed commit Original feedback: #267 (review) Automated run notes:
Validation:
|
THIS MESSAGE IS GENERATED BY AN AUTOMATED PROCESS.
Summary
computing-101/endian-escapades/zero-extensionbefore Sign Extension, with checker and private-test coverage for unsigned byte boundaries. Sign Extension now builds on that lesson, and both levels use the sharedcommon/harness-byte.c.computing-101/numbers-as-strings/divide-remainderto teach x86divseparately from buffer output.itoa-two-digits/DESCRIPTION.mdnow focuses on byte stores, whileitoa-digitand the new level sharecommon/harness-scalar.c.j2.computing-101/nibbling-on-numbers/mixed-conversions/DESCRIPTION.mdto name/challenge/mixed.linux-luminarium/text-editingmodule and its artifact-checkednano-basicschallenge before Chaining. The common image now installs Nano explicitly, andchaining/script/DESCRIPTION.mdlinks to the new lesson.linux-luminarium/users/su/DESCRIPTION.mdto explain that password entry displays neither characters nor asterisks.Validation
nix developtest run passed all 133 challenges and 146 test cases.build-frame, all affected number-to-string levels, Mixed Conversions, Nano, shell scripting, andsu.suauthentication with no password echo.git diff --check, andgit-cryptattribute checks passed.Notes / deferred follow-ups