judge: add multi-pass problem support#1167
Open
pandadtdyy wants to merge 14 commits into
Open
Conversation
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds multi-pass judging: ProblemConfigFile gains optional multi_pass; validation enforces interactor presence for interactive problems and multi_pass ∈ [2,20], and restricts default-problem multi-pass checkers to testlib/kattis. Checkers (testlib/kattis) can return nextPass (input and optional state). Default and interactive judges maintain per-case mp state, reuse mp.input/state across passes, annotate HYDRO_MULTI_PASS, and recursively re-run until max passes. UI adds a toggle and numeric input with YAML guards. Chinese locale adds "Max Passes". Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces “multi-pass” judging, allowing a problem to be evaluated across multiple sequential passes where a checker/interactor can emit the next-pass input (and optional state) until completion or a configured pass limit.
Changes:
- Add
multi_passto problem config types, editor serialization, Monaco YAML schema, and UI controls. - Extend default + interactive judges to support repeated execution passes and annotate result messages with pass context.
- Extend
testlibandkattischeckers to optionally emitnextpass.inandstate.txtartifacts for subsequent passes.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-default/locales/zh.yaml | Adds Chinese strings for multi-pass UI controls. |
| packages/ui-default/locales/en.yaml | Adds English strings for multi-pass UI controls. |
| packages/ui-default/components/problemconfig/ProblemType.tsx | Adds UI toggle + numeric input for configuring multi_pass. |
| packages/ui-default/components/problemconfig/ProblemConfigEditor.tsx | Includes multi_pass in YAML formatting/serialization (omits when disabled). |
| packages/ui-default/components/monaco/schema/problemconfig.ts | Adds multi_pass to Monaco YAML schema for editor validation. |
| packages/hydrojudge/src/judge/interactive.ts | Implements multi-pass support for interactive judging via interactor-emitted next input/state. |
| packages/hydrojudge/src/judge/default.ts | Implements multi-pass support for default judging via checker-emitted next input/state. |
| packages/hydrojudge/src/checkers.ts | Extends testlib/kattis checkers to return nextPass data from optional output files. |
| packages/hydrojudge/src/cases.ts | Adds server-side validation for multi_pass and interactive interactor requirements. |
| packages/common/types.ts | Adds multi_pass to the shared ProblemConfigFile type. |
Comments suppressed due to low confidence (1)
packages/hydrojudge/src/judge/default.ts:111
- The TLE rerun path calls
runner(ctx, ctxSubtask)without passing therunnerargument. With multi-pass enabled, the next pass relies on the third parameter (runner) for recursion; on a rerun this becomesundefinedandrunner!will crash when advancing to the next pass.
if (runner && ctx.rerun && c.time <= 5000 && status === STATUS.STATUS_TIME_LIMIT_EXCEEDED) {
ctx.rerun--;
return await runner(ctx, ctxSubtask);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/ui-default/components/problemconfig/ProblemConfigEditor.tsx (1)
53-53: ⚡ Quick winNormalize
multi_passby problem type during YAML formatting.Line 53 only checks the numeric value. Consider also guarding by
config.type(and default checker compatibility) so stalemulti_passdoesn’t leak into unsupported types.💡 Suggested change
- if (key === 'multi_pass' && (!config.multi_pass || config.multi_pass <= 1)) return; + if (key === 'multi_pass') { + const mp = Number(config.multi_pass || 0); + const allowed = config.type === 'interactive' + || (config.type === 'default' && ['testlib', 'kattis'].includes(config.checker_type as string)); + if (!allowed || mp <= 1) return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui-default/components/problemconfig/ProblemConfigEditor.tsx` at line 53, The YAML formatter currently only checks key === 'multi_pass' and numeric value (config.multi_pass) which can leave a stale multi_pass on problem types that don't support it; update the guard in ProblemConfigEditor (the branch handling key === 'multi_pass') to also check config.type and the problem's checker compatibility before allowing multi_pass to persist—i.e., if config.type is not a type that supports multi-pass or the selected checker is incompatible, treat multi_pass as absent (skip or delete it) otherwise keep it when >1; reference the existing key, config.multi_pass, and config.type checks to locate where to add the extra condition.packages/ui-default/components/problemconfig/ProblemType.tsx (1)
21-52: ⚡ Quick winGate default-type multi-pass behind compatible checker types.
For default problems, Line 147 currently shows multi-pass controls even when
checker_typeis nottestlib/kattis. That allows users to set invalid combinations that backend validation will reject.💡 Suggested change
- const multiPassEnabled = (multiPass || 0) > 1; + const defaultSupportsMultiPass = ['testlib', 'kattis'].includes(checkerType || ''); + const supportsMultiPass = Type === 'interactive' || (Type === 'default' && defaultSupportsMultiPass); + const multiPassEnabled = supportsMultiPass && (multiPass || 0) > 1; @@ - checked={multiPassEnabled} + checked={multiPassEnabled} + disabled={!supportsMultiPass} @@ - onChange={() => dispatch({ + onChange={() => supportsMultiPass && dispatch({ type: 'CONFIG_FORM_UPDATE', key: 'multi_pass', value: multiPassEnabled ? 0 : 2, - })} + })} @@ - {Type === 'default' && ( + {Type === 'default' && ( <div className="row"> {multiPassFields} </div> )}Also applies to: 147-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui-default/components/problemconfig/ProblemType.tsx` around lines 21 - 52, The multi-pass controls are currently shown for default problems regardless of checker_type; update the UI to only allow multi-pass for compatible checkers (testlib or kattis) by checking the form's checker_type before rendering or enabling controls. Concretely, in the component that defines multiPassFields use the existing multiPassEnabled and the checker_type value (from props/state) to change the conditions and behavior: only render the max-passes FormItem when multiPassEnabled && (checker_type === 'testlib' || checker_type === 'kattis'), and disable or no-op the Switch onChange when checker_type is not one of those values (still use dispatch with type 'CONFIG_FORM_UPDATE' and key 'multi_pass' when valid). Ensure you reference multiPassFields, multiPassEnabled, checker_type, dispatch and the 'CONFIG_FORM_UPDATE' action so the invalid combinations cannot be set in the UI.packages/ui-default/components/monaco/schema/problemconfig.ts (1)
83-83: ⚡ Quick winConstrain
multi_passto integer values in schema.Using
type: 'number'allows decimals in editor validation. This should beintegerto match pass-count semantics.💡 Suggested fix
- multi_pass: { type: 'number', minimum: 0, maximum: 10 }, + multi_pass: { type: 'integer', minimum: 0, maximum: 10 },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui-default/components/monaco/schema/problemconfig.ts` at line 83, The schema currently defines multi_pass with type: 'number', which allows decimals; update the multi_pass property to use type: 'integer' (keeping minimum: 0 and maximum: 10) so editor validation enforces whole pass counts—locate the multi_pass entry in problemconfig.ts and change the type to 'integer'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/hydrojudge/src/cases.ts`:
- Around line 35-37: The error message in the interactive validation check is
missing a space; update the thrown FormatError in the block that checks
config.type === 'interactive' and !config.interactor to use a corrected
user-facing string, e.g. "Interactive problems require an interactor." so
FormatError contains the fixed spacing and reads clearly to users.
- Around line 38-40: The validation for config.multi_pass currently only checks
range but allows fractional values; update the check where config.multi_pass is
validated (referencing config.multi_pass and the thrown FormatError) to also
require Number.isInteger(config.multi_pass) so non-integer values (e.g., 2.5)
trigger the same FormatError message "Multi Pass must be between 0 and 10.";
ensure the combined condition rejects null/undefined appropriately and still
enforces 0 ≤ multi_pass ≤ 10.
In `@packages/hydrojudge/src/judge/default.ts`:
- Around line 33-35: When multiPass is active, route the per-pass state
(mp.stdin, mp.copyIn, mp.input) to the checker instead of mounting it into the
user sandbox: change the places that build the user process execution object
(the fields stdin and copyIn built from multiPass ? mp.stdin : ... and multiPass
? { ...ctx.execute.copyIn, ...mp.copyIn } : ctx.execute.copyIn) so the user
always gets the original run inputs (c.input and ctx.execute.copyIn), while the
checker invocation receives mp.stdin, mp.copyIn merged with ctx.execute.copyIn,
and uses mp.input (not c.input) for later passes; update all occurrences that
construct the checker call (where c.input and original copyIn are passed) to use
mp.* when multiPass is true.
In `@packages/hydrojudge/src/judge/interactive.ts`:
- Line 35: The current multi-pass logic passes mp.copyIn into the user sandbox
(copyIn: multiPass && mp.i ? { ...ctx.executeUser.copyIn, ...mp.copyIn } :
ctx.executeUser.copyIn), which leaks the interactor's state.txt; change the
merge so state.txt is excluded before supplying mp.copyIn to the user process
(e.g., create a filtered copy of mp.copyIn that removes any entry for
"state.txt" or only includes allowed files), and use that filtered object in
place of mp.copyIn when computing copyIn; update all similar sites (the other
copyIn usage at the same pattern and where mp.copyIn is constructed) so
mp.copyIn never exposes state.txt to the user sandbox.
---
Nitpick comments:
In `@packages/ui-default/components/monaco/schema/problemconfig.ts`:
- Line 83: The schema currently defines multi_pass with type: 'number', which
allows decimals; update the multi_pass property to use type: 'integer' (keeping
minimum: 0 and maximum: 10) so editor validation enforces whole pass
counts—locate the multi_pass entry in problemconfig.ts and change the type to
'integer'.
In `@packages/ui-default/components/problemconfig/ProblemConfigEditor.tsx`:
- Line 53: The YAML formatter currently only checks key === 'multi_pass' and
numeric value (config.multi_pass) which can leave a stale multi_pass on problem
types that don't support it; update the guard in ProblemConfigEditor (the branch
handling key === 'multi_pass') to also check config.type and the problem's
checker compatibility before allowing multi_pass to persist—i.e., if config.type
is not a type that supports multi-pass or the selected checker is incompatible,
treat multi_pass as absent (skip or delete it) otherwise keep it when >1;
reference the existing key, config.multi_pass, and config.type checks to locate
where to add the extra condition.
In `@packages/ui-default/components/problemconfig/ProblemType.tsx`:
- Around line 21-52: The multi-pass controls are currently shown for default
problems regardless of checker_type; update the UI to only allow multi-pass for
compatible checkers (testlib or kattis) by checking the form's checker_type
before rendering or enabling controls. Concretely, in the component that defines
multiPassFields use the existing multiPassEnabled and the checker_type value
(from props/state) to change the conditions and behavior: only render the
max-passes FormItem when multiPassEnabled && (checker_type === 'testlib' ||
checker_type === 'kattis'), and disable or no-op the Switch onChange when
checker_type is not one of those values (still use dispatch with type
'CONFIG_FORM_UPDATE' and key 'multi_pass' when valid). Ensure you reference
multiPassFields, multiPassEnabled, checker_type, dispatch and the
'CONFIG_FORM_UPDATE' action so the invalid combinations cannot be set in the UI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 505d376f-e590-4802-b326-2880b3060277
📒 Files selected for processing (10)
packages/common/types.tspackages/hydrojudge/src/cases.tspackages/hydrojudge/src/checkers.tspackages/hydrojudge/src/judge/default.tspackages/hydrojudge/src/judge/interactive.tspackages/ui-default/components/monaco/schema/problemconfig.tspackages/ui-default/components/problemconfig/ProblemConfigEditor.tsxpackages/ui-default/components/problemconfig/ProblemType.tsxpackages/ui-default/locales/en.yamlpackages/ui-default/locales/zh.yaml
Member
Author
|
I have read the CLA Document and I hereby sign the CLA |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/hydrojudge/src/cases.ts (1)
38-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClose the validation gap at
multi_pass = 1.The current logic allows
multi_pass = 1to pass validation (integer, within 0–10) without triggering the testlib/kattis checker requirement (line 41 checks> 1). However, the UI only exposes 0 (disabled) or 2–10 (enabled), makingmulti_pass = 1an inaccessible and ambiguous state.Tighten the validation to match the UI constraints: reject
multi_pass = 1or clarify whether it has distinct semantics.Suggested fix
- if (config.multi_pass && (!Number.isInteger(config.multi_pass) || config.multi_pass < 0 || config.multi_pass > 10)) { - throw new FormatError('Multi Pass must be between 0 and 10.'); + if (config.multi_pass && (!Number.isInteger(config.multi_pass) || config.multi_pass < 0 || config.multi_pass === 1 || config.multi_pass > 10)) { + throw new FormatError('Multi Pass must be 0 or an integer between 2 and 10.'); } if (config.multi_pass > 1 && config.type === 'default' && !['testlib', 'kattis'].includes(config.checker_type)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/hydrojudge/src/cases.ts` around lines 38 - 43, The validation currently allows multi_pass = 1, which is unsupported by the UI; update the validation around config.multi_pass in the conditional that throws FormatError so that 1 is rejected (i.e., only allow 0 or 2–10). Modify the first if that checks config.multi_pass to also treat 1 as invalid (or change the numeric range check to exclude 1), keeping the subsequent checker requirement for multi_pass > 1 intact; reference config.multi_pass and the FormatError throw in this file to locate and fix the logic.
🧹 Nitpick comments (1)
packages/ui-default/components/problemconfig/ProblemConfigForm.tsx (1)
50-53: ⚡ Quick winClarify toggle logic to avoid relying on stale closure.
The current code depends on
enabledbeing stale whendispatchruns (sincesetEnabledis async). Capture the new value explicitly to make the intent clear and reduce fragility.Refactor for clarity
onChange={() => { - setEnabled(!enabled); - dispatch({ type: 'CONFIG_FORM_UPDATE', key: 'multi_pass', value: enabled ? 0 : 2 }); + const newEnabled = !enabled; + setEnabled(newEnabled); + dispatch({ type: 'CONFIG_FORM_UPDATE', key: 'multi_pass', value: newEnabled ? 2 : 0 }); }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui-default/components/problemconfig/ProblemConfigForm.tsx` around lines 50 - 53, The toggle handler relies on the old enabled value due to setEnabled being async; capture the new boolean first (e.g., newEnabled = !enabled), call setEnabled(newEnabled), then dispatch({ type: 'CONFIG_FORM_UPDATE', key: 'multi_pass', value: newEnabled ? 2 : 0 }) so the dispatched value uses the explicit new state instead of the possibly stale enabled closure; update the handler around setEnabled/dispatch in ProblemConfigForm.tsx (references: setEnabled, enabled, dispatch, CONFIG_FORM_UPDATE, key 'multi_pass').
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/hydrojudge/src/judge/default.ts`:
- Around line 67-72: The multi-pass path mutates mp and calls runner!(...) but
the TLE retry branch does not forward the recursive callback, so retries can
resume with runner === undefined and crash; update the TLE retry code to accept
and pass the same runner callback through (preserve runner when creating
ctxSubtask/retrying and when re-invoking the judge/rerun logic) so that when
nextPass is returned the subsequent call uses the original runner (refer to
symbols mp, nextPass, runner, ctxSubtask and the retry/TLE branch in
default.ts).
In `@packages/ui-default/components/problemconfig/ProblemConfigForm.tsx`:
- Line 57: The FormItem in ProblemConfigForm is passing a translated string via
i18n('Max passes') which causes double translation because FormItem (see
BasicForm's label handling) already wraps the label with i18n; fix this by
removing the i18n call and pass the raw label key directly (e.g., change
label={i18n('Max passes')} to label={'Max passes'} or the project's canonical
raw key) on the FormItem component so only one translation is applied.
---
Outside diff comments:
In `@packages/hydrojudge/src/cases.ts`:
- Around line 38-43: The validation currently allows multi_pass = 1, which is
unsupported by the UI; update the validation around config.multi_pass in the
conditional that throws FormatError so that 1 is rejected (i.e., only allow 0 or
2–10). Modify the first if that checks config.multi_pass to also treat 1 as
invalid (or change the numeric range check to exclude 1), keeping the subsequent
checker requirement for multi_pass > 1 intact; reference config.multi_pass and
the FormatError throw in this file to locate and fix the logic.
---
Nitpick comments:
In `@packages/ui-default/components/problemconfig/ProblemConfigForm.tsx`:
- Around line 50-53: The toggle handler relies on the old enabled value due to
setEnabled being async; capture the new boolean first (e.g., newEnabled =
!enabled), call setEnabled(newEnabled), then dispatch({ type:
'CONFIG_FORM_UPDATE', key: 'multi_pass', value: newEnabled ? 2 : 0 }) so the
dispatched value uses the explicit new state instead of the possibly stale
enabled closure; update the handler around setEnabled/dispatch in
ProblemConfigForm.tsx (references: setEnabled, enabled, dispatch,
CONFIG_FORM_UPDATE, key 'multi_pass').
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e9f84f2-d496-4d34-8268-ed77fd459393
📒 Files selected for processing (9)
packages/hydrojudge/src/cases.tspackages/hydrojudge/src/checkers.tspackages/hydrojudge/src/judge/default.tspackages/hydrojudge/src/judge/interactive.tspackages/hydrojudge/src/judge/interface.tspackages/ui-default/components/monaco/schema/problemconfig.tspackages/ui-default/components/problemconfig/ProblemConfigEditor.tsxpackages/ui-default/components/problemconfig/ProblemConfigForm.tsxpackages/ui-default/locales/zh.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/hydrojudge/src/judge/default.ts`:
- Line 72: The code calls the optional parameter runner without guarding (return
await runner(ctx, ctxSubtask, runner);) which can cause a TypeError; update the
call site in default.ts to either (a) check that runner is defined before
invoking it (e.g., if (!runner) handle/throw or return a sensible fallback) or
(b) if the architecture guarantees runner is always present for multi-pass
problems, use a non-null assertion to call runner! to match interactive.ts;
reference the symbol runner and the recursive call in default.ts when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8d73608-a49d-44b3-a0f6-3b13d96f4e4e
📒 Files selected for processing (4)
packages/hydrojudge/src/cases.tspackages/hydrojudge/src/judge/default.tspackages/hydrojudge/src/judge/interactive.tspackages/ui-default/components/problemconfig/ProblemConfigForm.tsx
| mp.input = nextPass.input; | ||
| mp.state = nextPass.state ?? undefined; | ||
| mp.i++; | ||
| return await runner(ctx, ctxSubtask, runner); |
There was a problem hiding this comment.
Missing null guard on runner before recursive call.
runner is typed as optional (runner?: Function), but line 72 calls it without checking. Line 78 correctly guards with if (runner && ...). If nextPass is present but the caller didn't supply runner, this will throw a TypeError at runtime.
Proposed fix
if (nextPass) {
if (mp.i < ctx.config.multi_pass) {
mp.input = nextPass.input;
mp.state = nextPass.state ?? undefined;
mp.i++;
+ if (!runner) {
+ return {
+ id: c.id,
+ subtaskId: ctxSubtask.subtask.id,
+ status: STATUS.STATUS_SYSTEM_ERROR,
+ score: 0,
+ time,
+ memory,
+ message: 'Multi-pass requires runner callback',
+ };
+ }
return await runner(ctx, ctxSubtask, runner);Alternatively, use the non-null assertion (runner!) to match interactive.ts if the architecture guarantees runner is always provided for multi-pass problems.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return await runner(ctx, ctxSubtask, runner); | |
| if (nextPass) { | |
| if (mp.i < ctx.config.multi_pass) { | |
| mp.input = nextPass.input; | |
| mp.state = nextPass.state ?? undefined; | |
| mp.i++; | |
| if (!runner) { | |
| return { | |
| id: c.id, | |
| subtaskId: ctxSubtask.subtask.id, | |
| status: STATUS.STATUS_SYSTEM_ERROR, | |
| score: 0, | |
| time, | |
| memory, | |
| message: 'Multi-pass requires runner callback', | |
| }; | |
| } | |
| return await runner(ctx, ctxSubtask, runner); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/hydrojudge/src/judge/default.ts` at line 72, The code calls the
optional parameter runner without guarding (return await runner(ctx, ctxSubtask,
runner);) which can cause a TypeError; update the call site in default.ts to
either (a) check that runner is defined before invoking it (e.g., if (!runner)
handle/throw or return a sensible fallback) or (b) if the architecture
guarantees runner is always present for multi-pass problems, use a non-null
assertion to call runner! to match interactive.ts; reference the symbol runner
and the recursive call in default.ts when making the change.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/hydrojudge/src/cases.ts (1)
38-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject falsy invalid
multi_passvalues in backend validation.The truthy guard allows invalid provided values like
0to bypass this check. Validate on presence (!= null) instead, so every provided value must satisfy integer + range constraints.💡 Suggested fix
- if (config.multi_pass && (!Number.isInteger(config.multi_pass) || config.multi_pass < 2 || config.multi_pass > 20)) { + if (config.multi_pass != null && (!Number.isInteger(config.multi_pass) || config.multi_pass < 2 || config.multi_pass > 20)) { throw new FormatError('Multi Pass must be between 2 and 20.'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/hydrojudge/src/cases.ts` around lines 38 - 39, The current validation uses a truthy guard on config.multi_pass which lets falsy but provided values (e.g., 0) bypass checks; change the conditional to check presence with != null (or !== undefined) and then enforce Number.isInteger(config.multi_pass) and the range 2..20, throwing FormatError('Multi Pass must be between 2 and 20.') when the provided value is invalid; update the validation around config.multi_pass in the same block where FormatError is thrown to ensure any provided value fails unless it is an integer within the allowed range.packages/hydrojudge/src/judge/default.ts (1)
74-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd null assertion or guard on
runnerbefore recursive call.
runneris typed as optional (runner?: Function), but line 74 calls it directly without a check. IfnextPassis present but the caller didn't supplyrunner, this will throwTypeErrorat runtime. The equivalent call ininteractive.tsusesrunner!(non-null assertion).For consistency and to document the assumption that
runneris always provided for multi-pass problems, use the non-null assertion:- return await runner(ctx, ctxSubtask, runner); + return await runner!(ctx, ctxSubtask, runner);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/hydrojudge/src/judge/default.ts` at line 74, The call to runner in default.ts (return await runner(ctx, ctxSubtask, runner)) uses the optional parameter runner?: Function without a null check; change it to use a non-null assertion (runner!) like in interactive.ts so the recursive call becomes return await runner!(ctx, ctxSubtask, runner) — this documents the invariant that a runner must be supplied for multi-pass problems and avoids a potential TypeError at runtime.packages/hydrojudge/src/judge/interactive.ts (1)
19-19:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAvoid leaking interactor state to user sandbox.
Line 19 merges
mp.stateinto the user processcopyIn. Sincemp.statecontainsstate.txt(from lines 69-71), this mounts the interactor's internal state file into the user sandbox across passes. This could leak privileged interactor information to submissions.The user process should not receive
mp.state; only the interactor needs it:- copyIn: { ...ctx.executeUser.copyIn, ...mp.state }, + copyIn: ctx.executeUser.copyIn,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/hydrojudge/src/judge/interactive.ts` at line 19, The current merge of mp.state into the user process copyIn leaks interactor internals (mp.state/state.txt) into the sandbox; remove mp.state from being merged into ctx.executeUser.copyIn and instead ensure mp.state is only passed to the interactor process (e.g., merge mp.state into the interactor's copyIn/copyOut or the data structure used by the interactor runner); update the code around the identifiers copyIn, ctx.executeUser.copyIn and mp.state in interactive.ts so that user execution receives only ctx.executeUser.copyIn (and other safe mounts) while mp.state is kept private to the interactor.
🧹 Nitpick comments (1)
packages/ui-default/components/monaco/schema/problemconfig.ts (1)
83-83: ⚡ Quick winMirror backend multi-pass checker constraints in schema.
multi_passrange is correct, but the schema still misses the backend rule for default problems (checker_typemust betestlib/kattiswhen multi-pass is set). Adding anif/thenconstraint keeps UI and server validation aligned.💡 Suggested schema addition
const problemConfigSchema: JSONSchema7 = { type: 'object', + allOf: [ + { + if: { + properties: { + type: { const: 'default' }, + multi_pass: { type: 'integer', minimum: 2 }, + }, + required: ['type', 'multi_pass'], + }, + then: { + properties: { + checker_type: { enum: ['testlib', 'kattis'] }, + }, + required: ['checker_type'], + }, + }, + ], definitions: {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui-default/components/monaco/schema/problemconfig.ts` at line 83, Add a JSON Schema conditional to enforce that when multi_pass is present/greater than 1 the checker_type must be one of the backend-allowed values for default problems (i.e., "testlib" or "kattis"); modify the schema around the multi_pass entry to include an if: { required: ["multi_pass"] } (or if: { properties: { multi_pass: { minimum: 2 } } }) and then: { required: ["checker_type"], properties: { checker_type: { enum: ["testlib","kattis"] } } } so the UI validation for multi_pass aligns with the backend rule that checker_type must be testlib/kattis.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/hydrojudge/src/cases.ts`:
- Around line 38-39: The current validation uses a truthy guard on
config.multi_pass which lets falsy but provided values (e.g., 0) bypass checks;
change the conditional to check presence with != null (or !== undefined) and
then enforce Number.isInteger(config.multi_pass) and the range 2..20, throwing
FormatError('Multi Pass must be between 2 and 20.') when the provided value is
invalid; update the validation around config.multi_pass in the same block where
FormatError is thrown to ensure any provided value fails unless it is an integer
within the allowed range.
In `@packages/hydrojudge/src/judge/default.ts`:
- Line 74: The call to runner in default.ts (return await runner(ctx,
ctxSubtask, runner)) uses the optional parameter runner?: Function without a
null check; change it to use a non-null assertion (runner!) like in
interactive.ts so the recursive call becomes return await runner!(ctx,
ctxSubtask, runner) — this documents the invariant that a runner must be
supplied for multi-pass problems and avoids a potential TypeError at runtime.
In `@packages/hydrojudge/src/judge/interactive.ts`:
- Line 19: The current merge of mp.state into the user process copyIn leaks
interactor internals (mp.state/state.txt) into the sandbox; remove mp.state from
being merged into ctx.executeUser.copyIn and instead ensure mp.state is only
passed to the interactor process (e.g., merge mp.state into the interactor's
copyIn/copyOut or the data structure used by the interactor runner); update the
code around the identifiers copyIn, ctx.executeUser.copyIn and mp.state in
interactive.ts so that user execution receives only ctx.executeUser.copyIn (and
other safe mounts) while mp.state is kept private to the interactor.
---
Nitpick comments:
In `@packages/ui-default/components/monaco/schema/problemconfig.ts`:
- Line 83: Add a JSON Schema conditional to enforce that when multi_pass is
present/greater than 1 the checker_type must be one of the backend-allowed
values for default problems (i.e., "testlib" or "kattis"); modify the schema
around the multi_pass entry to include an if: { required: ["multi_pass"] } (or
if: { properties: { multi_pass: { minimum: 2 } } }) and then: { required:
["checker_type"], properties: { checker_type: { enum: ["testlib","kattis"] } } }
so the UI validation for multi_pass aligns with the backend rule that
checker_type must be testlib/kattis.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56cb3a6e-8506-40b7-bba6-48a221047631
📒 Files selected for processing (4)
packages/hydrojudge/src/cases.tspackages/hydrojudge/src/judge/default.tspackages/hydrojudge/src/judge/interactive.tspackages/ui-default/components/monaco/schema/problemconfig.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/hydrojudge/src/judge/default.ts (1)
65-68: 💤 Low valueSignal message loses pass prefix.
When a signal is caught (line 66), the message is overwritten with
signals[code], losing the[Pass N]prefix. The non-signal branch (line 67) preserves the prefix. Consider prepending the pass info for consistency.Suggested fix
- if (code < 32 && signalled) message = signals[code]; + if (code < 32 && signalled) message = `${message}${signals[code]}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/hydrojudge/src/judge/default.ts` around lines 65 - 68, The runtime-error branch for STATUS.STATUS_RUNTIME_ERROR currently overwrites message with signals[code] when signalled, losing the original "[Pass N]" prefix; update the signalled branch in the same conditional (where variables code, signalled, message, and signals are used and ctx.config.detail === 'full') to preserve the pass prefix by prepending the existing message/prefix to signals[code] (or by constructing an object similar to the non-signalled branch that combines the prefix and the signal text), so the final message retains the pass info plus the signal description.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/hydrojudge/src/judge/default.ts`:
- Around line 65-68: The runtime-error branch for STATUS.STATUS_RUNTIME_ERROR
currently overwrites message with signals[code] when signalled, losing the
original "[Pass N]" prefix; update the signalled branch in the same conditional
(where variables code, signalled, message, and signals are used and
ctx.config.detail === 'full') to preserve the pass prefix by prepending the
existing message/prefix to signals[code] (or by constructing an object similar
to the non-signalled branch that combines the prefix and the signal text), so
the final message retains the pass info plus the signal description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e14d2da1-64ec-40cd-8972-9cd321d26d49
📒 Files selected for processing (1)
packages/hydrojudge/src/judge/default.ts
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
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.
Summary by CodeRabbit
New Features
Validation
Chores