Scripts/make arxiv #9
Conversation
|
Warning Rate limit exceeded@flyingrobots has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntegrates arXiv tarball production into paper CI: workflows now build/validate/upload Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Runner as CI Runner
participant Script as make-arxiv.sh
participant FS as Working Dir (tmp)
participant Artifact as GitHub Artifacts / Release
GH->>Runner: start build-papers/release-paper workflow
Runner->>Script: execute make-arxiv.sh
Script->>FS: find & copy files (flatten), check duplicates
alt duplicates found
Script->>Runner: exit non-zero (fail)
Runner->>GH: mark step failed
else success
Script->>FS: filter .tex lines, refresh main.bbl, create ax.tar
Runner->>Artifact: upload ax.tar artifact
Runner->>GH: set output tar_present=true
GH->>Artifact: include ax.tar in GitHub Release (gated on tar_present)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
aion-holography/intro.tex (2)
5-26: Opening and holography bullet: sharpen micro‑consistency and precisionThe high‑level story reads well, but a couple of small, consequential nits:
- Line 6: “distributed, multi-core, and AI-mediated” — check that “multi-core” matches your house spelling elsewhere (“multicore”, “multi-core”, etc.). Right now you have similar micro‑choices (e.g. “double--pushout”) that you’re clearly being careful about; this one should be consistent too.
- Lines 23–25: “the entire interior evolution of a computation is stored … yielding an information-complete holographic encoding.” This is conceptually mirrored in the abstract (“internal evolution … encoded on the boundary”) and in the holography contribution (“interior evolution … derivation volume”). Decide whether your canonical phrase is “internal evolution” or “interior evolution” (and possibly “derivation volume”), then normalize that across abstract, intro bullet, and the theorem statement to avoid readers wondering if you mean subtly different things.
Content-wise this bullet matches the later holography theorem, but the terminology drift is avoidable.
61-80: Based on my verification against the formal theorem and corollary statements, I've identified issues in each bullet that need correction:Bullet 2 (Two-plane commutation): The theorem statement proves the commutation via a commutative square diagram, but the fibration structure is discussed in a subsequent subsection for structural justification, not claimed in the theorem itself. The bullet overstates the theorem's claim.
Bullet 3 (Worldline uniqueness): The corollary uses language like "RMG successor" and "interior derivation uniquely up to isomorphism," not "complete derivation worldlines." The terminology is tighter in the corollary than the bullet suggests.
Bullet 5 (Rulial distance): The theorem proves that
$D_{\tau,m}$ satisfies the triangle inequality property (making it a quasi-pseudometric). While the definition of$D_{\tau,m}$ frames it as an MDL-based distance measuring translation complexity, the theorem itself only establishes the triangle inequality—the bullet conflates the definition's interpretation with what the theorem formally proves.
Key contributions bullets: verify wording aligns with formal statements
Bullet 2: Remove "via a fibration structure"—Theorem~\ref{thm:two-plane} proves the commutation directly; the fibration is a structural justification introduced afterward. Revise to: "attachment and skeleton updates commute up to isomorphism".
Bullet 3: Replace "complete derivation worldlines" with terminology from the corollary. Corollary~\ref{cor:worldline-uniqueness} states that boundary data
$(S_0,P)$ "determines the interior derivation uniquely up to isomorphism," not "complete worldlines." Revise to: "interior derivation is uniquely determined up to isomorphism by the boundary data, extending per-tick determinism to whole-run semantics".Bullet 5: Clarify scope. Theorem~\ref{thm:rulial-triangle} proves the triangle inequality property; it does not itself characterize what the distance "quantifies." The characterization comes from the definition of
$D_{\tau,m}$ . Revise to: "the triangle inequality property of an MDL-based quasi-pseudometric on observers, capturing the complexity of translating between different computational views".
♻️ Duplicate comments (2)
.github/workflows/release-paper.yml (1)
28-29: Script path is still incorrect—will fail with "No such file or directory".Line 29 references
scripts/make-arxiv.sh, but the script is located ataion-holography/scripts/make-arxiv.sh. This is the same path error flagged in previous review comments; despite that feedback, the path remains broken. The pipeline will fail immediately (exit code 127) when this step runs, blocking the release.Apply this fix:
- - name: Build arXiv tarball - run: scripts/make-arxiv.sh + - name: Build arXiv tarball + run: aion-holography/scripts/make-arxiv.sh.github/workflows/build-papers.yml (1)
27-28: Script path is incorrect—will fail with "No such file or directory".Line 28 references
scripts/make-arxiv.sh, but the script is located ataion-holography/scripts/make-arxiv.sh. This is a duplicate of the prior review feedback; the path error remains unaddressed. This blocks all builds on main, PRs, and releases (exit code 127).Apply this fix:
- - name: Build arXiv tarball - run: scripts/make-arxiv.sh + - name: Build arXiv tarball + run: aion-holography/scripts/make-arxiv.sh
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
-
.github/workflows/build-papers.yml(2 hunks) -
.github/workflows/release-paper.yml(2 hunks) -
aion-holography/assumptions.tex(1 hunks) -
aion-holography/intro.tex(4 hunks) -
aion-holography/main.tex(1 hunks) -
aion-holography/scripts/make-arxiv.sh(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build LaTeX Papers
aion-holography/scripts/make-arxiv.sh
[error] 1-1: scripts/make-arxiv.sh: No such file or directory. The step 'Run scripts/make-arxiv.sh' failed (exit code 127).
🔇 Additional comments (7)
.github/workflows/release-paper.yml (1)
54-56: Verify ax.tar is generated before release upload.The release step attempts to upload
ax.tar(line 56), which is generated by the "Build arXiv tarball" step (lines 28–29). Since that step has a path error and will fail, this file will not exist, causing the GitHub Release creation to fail. Once the script path is corrected, this section should work as intended..github/workflows/build-papers.yml (2)
37-42: Artifact upload configuration is sound but depends on prior step fix.Once the "Build arXiv tarball" step path is corrected, this artifact upload should work as intended.
54-56: Release file list is correctly formatted.The multi-line format is valid. Once the arXiv tarball generation step is fixed, this should upload both the PDF and tarball to the release.
aion-holography/scripts/make-arxiv.sh (4)
1-26: Script preamble and path resolution are sound.The ROOT path calculation correctly navigates from
aion-holography/scripts/make-arxiv.shup two levels to the repo root. The script will need to be invoked via the correct path (aion-holography/scripts/make-arxiv.sh) from the workflows for this logic to execute.
28-37: File collection logic is sound, but verify pipe exit propagation.The find-based collection with null-delimited processing is the right approach for handling filenames with special characters. Duplicate detection is correct and intentional (arXiv requires flat structure).
However, there is a subtle bash trap here: the while loop runs in a subshell (due to the pipe). When
exit 1is called inside the subshell (line 34), it only exits that subshell, not the parent script. Althoughset -eis active, exit codes from subshells in pipes are not always propagated reliably across all bash versions—a safer pattern would check the exit status after the loop or use explicit error propagation.Verify that the script terminates correctly when a duplicate is detected by testing this case:
- Create test source with duplicate filenames in different subdirs (e.g.,
sub1/config.texandsub2/config.tex)- Run the script and confirm it exits with status 1
- Confirm no partial ax.tar is created
Consider using this safer pattern:
find "${SRC}" -type f \( -name 'Makefile' -o -name '*.tex' -o -name 'main.bbl' \) -print0 \ | while IFS= read -r -d '' f; do dest="${WORKDIR}/$(basename "$f")" if [[ -e "${dest}" ]]; then echo "Duplicate filename detected while flattening: $(basename "$f")" >&2 exit 1 fi cp "$f" "$dest" done || exit 1The
|| exit 1after the loop ensures the script exits if the subshell fails.
39-42: Potential regex bug: Pattern may not match leading whitespace as intended.Line 41 uses the perl pattern
/^\\s*%/inside single quotes. In shell single quotes,\\is treated as two literal backslash characters, which means perl receives\\s(backslash followed by 's'), not the whitespace character class\s. This would match a literal backslash-s sequence, not leading whitespace.The comment on line 39 states the intent is to "Strip full-line comments from .tex sources (keep inline %)", which suggests the pattern should match lines starting with optional whitespace followed by
%. If that is the intent, the regex should be fixed to correctly escape the backslash:Test whether the current regex works for stripping comments by running:
#!/bin/bash # Create a test .tex file with various comment styles cat > /tmp/test_comments.tex <<'EOF' % Full-line comment % Indented comment Some text here % inline comment EOF # Test the perl command as written in the script perl -ni -e 'next if /^\\s*%/; print' /tmp/test_comments.tex cat /tmp/test_comments.texExpected output if regex is correct: Only "Some text here % inline comment" line remains. If the regex is buggy, the full-line comments will still be present.
If the test shows the regex does NOT work correctly, apply this fix:
- perl -ni -e 'next if /^\\s*%/; print' "$f" + perl -ni -e 'next if /^\s*%/; print' "$f"(Use a single backslash
\sinstead of double\\sin the single-quoted string.)
44-46: Tar creation and final cleanup look good.The tar command correctly changes to the flattened working directory and creates the archive at the repo root. The temp directory cleanup via trap (line 26) ensures no artifacts are left behind.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build-papers.yml(2 hunks).github/workflows/release-paper.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-holography
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
aion-holography/main.tex (1)
71-85: Terminology collision: "double-pushout graph rewriting" here vs. "double-pushout (DPO) approach" in intro.The abstract now says "double-pushout graph rewriting in adhesive categories" (line 74), but intro.tex line 28-29 still uses "double-pushout (DPO) approach in adhesive categories."
Either:
- Normalize both to "double-pushout (DPO) graph rewriting", or
- Pick one canonical phrase and use it in both locations.
The inconsistency undermines the professional polish of the rewrite.
Per the previous review, this terminology drift was already flagged. It remains unresolved.
aion-holography/intro.tex (1)
28-29: Terminology mismatch with abstract: "approach" vs. "graph rewriting".You split the sentence (addressing the previous nitpick), but line 28-29 says:
"double-pushout (DPO) approach in adhesive categories"
while main.tex line 74 says:
"double-pushout graph rewriting in adhesive categories"
Pick one house style for the entire paper:
- Either "double-pushout (DPO) approach" everywhere, or
- "double-pushout (DPO) graph rewriting" everywhere.
Mixing them across abstract and intro is sloppy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
aion-holography/determinism_confluence.tex(1 hunks)aion-holography/dpo_rmg.tex(1 hunks)aion-holography/intro.tex(3 hunks)aion-holography/main.tex(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-holography
🔇 Additional comments (7)
aion-holography/dpo_rmg.tex (1)
5-6: LGTM — scope qualifier is helpful.Adding "briefly review" appropriately signals the level of detail readers should expect in this section.
aion-holography/intro.tex (6)
6-6: Good fix — "multicore" is the standard spelling.
23-25: Cleaner phrasing — "information-complete holographic encoding" is direct and precise.
39-41: Confluence wording improved, but verify against Theorem~\ref{thm:global}.You added "under the standing assumptions and standard rewrite-theoretic conditions" to qualify the global confluence claim. This is more careful than the previous version.
However, ensure that "standard rewrite-theoretic conditions" accurately covers what Theorem~\ref{thm:global} requires (joinable critical pairs + termination/decreasing diagrams). If those conditions go beyond what's typically "standard," consider being explicit: "under joinable critical pairs and termination" or similar.
This addresses the previous nitpick's concern about overselling, but precision matters at this level.
50-57: AION stack scope is now explicit — well done.The new wording clearly delineates:
- What is in scope: axioms and invariants in Sections \ref{sec:rmg}--\ref{sec:discussion}
- What is out of scope: scheduling, representation, persistence details
- Where the rest goes: companion "COMPUTER" paper
This directly addresses the previous concern about vagueness. Much tighter.
68-69: Simplified commutation wording is cleaner.Condensing to "commute up to isomorphism" keeps the intro tight while the full fibration machinery stays in Theorem~\ref{thm:two-plane}.
70-73: Worldline uniqueness wording is precise and well-connected."uniquely determined up to typed open graph isomorphism by the boundary data, extending per-tick commutation to whole-run semantics" — this ties the corollary back to the per-tick results cleanly while being technically exact.
|
|
||
| Throughout this section we work under the standing assumptions | ||
| summarised in \cref{sec:assumptions}. | ||
| summarised in the Standing Assumptions section. |
There was a problem hiding this comment.
Revert to \cref{sec:assumptions} instead of hardcoded text.
Replacing \cref{sec:assumptions} with plain text "the Standing Assumptions section" breaks three things:
- Hyperlink navigation — readers lose the clickable cross-reference.
- Automatic numbering — if the section is renumbered or moved, this reference becomes stale.
- Consistency — the rest of the paper uses
\crefthroughout (e.g., line 10, line 189).
Apply this diff to restore best practice:
-summarised in the Standing Assumptions section.
+summarised in \cref{sec:assumptions}.📝 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.
| summarised in the Standing Assumptions section. | |
| summarised in \cref{sec:assumptions}. |
🤖 Prompt for AI Agents
In aion-holography/determinism_confluence.tex around line 6, the plain text "the
Standing Assumptions section" replaced the cross-reference and broke
hyperlinking, automatic numbering, and consistency; revert that text back to
\cref{sec:assumptions} so the document uses a clickable, automatically numbered
cross-reference consistent with the rest of the paper.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-papers.yml (1)
19-31: You’re running LaTeX twice in this job; consider collapsing to a single build
latex-actionbuildsaion-holography/main.texfirst, and thenmake-arxiv.shrunslatexmk -pdf main.texagain to refreshmain.bbl. That’s correct but wasteful on every push/PR.Options to de-duplicate:
- Let
make-arxiv.shbe the single source of truth for building bothmain.pdfandax.tar, and drop the separateCompile LaTeXstep here; or- Teach
make-arxiv.shto skip itslatexmkinvocation whenmain.bblis already up-to-date (or accept an env var/flag from the workflow to skip the rebuild).Not a correctness bug, but it’s unnecessary extra work on every CI run.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build-papers.yml(1 hunks).github/workflows/release-paper.yml(3 hunks)aion-holography/intro.tex(3 hunks)aion-holography/main.tex(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-holography
🔇 Additional comments (1)
aion-holography/main.tex (1)
70-85: Abstract wording is coherent with the body and prior nits look resolvedThe new abstract tracks the actual payload well: DPO graph rewriting, RMGs, deterministic concurrent semantics, tick-level + two-plane confluence, conditional global confluence, holography on a single edge, and MDL-based rulial distance. Terminology (“double-pushout (DPO) graph rewriting”, “interior evolution”, “provenance payload”, “quasi-pseudometric”) now matches the intro/contributions list and doesn’t appear to overstate any theorem-level claim.
No changes requested here.
| confluence results: tick-level determinism and two-plane | ||
| commutation (Theorems~\ref{thm:tick-confluence} and | ||
| \ref{thm:two-plane}), and, under standard rewrite-theory | ||
| hypotheses, global confluence (Theorem~\ref{thm:global}); | ||
| \item a provenance payload calculus giving \emph{computational | ||
| \ref{thm:two-plane}), and---under the standing assumptions and | ||
| standard rewrite-theoretic conditions---global confluence | ||
| (Theorem~\ref{thm:global}); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Spell out or cross-reference the exact hypotheses behind “global confluence”
“under the standing assumptions and standard rewrite-theoretic conditions—global confluence (Theorem~\textbackslash ref{thm:global})” is still pretty opaque at intro level. Right now a reader has no idea whether that means “termination + local confluence on the skeleton” or something more bespoke.
I’d either (a) add an explicit pointer to where those conditions are enumerated (table/section/definition), or (b) mirror the theorem’s statement more literally here so you’re not hand-waving the hypotheses in the most prominent summary paragraph.
🤖 Prompt for AI Agents
In aion-holography/intro.tex around lines 37 to 41, the phrase "under the
standing assumptions and standard rewrite-theoretic conditions—global confluence
(Theorem~\ref{thm:global})" is too vague for the introduction; either add an
explicit cross-reference to the exact place those hypotheses are listed (e.g.,
"see Section X / Definition Y / Table Z for assumptions") or briefly mirror the
theorem's hypothesis list in one sentence (e.g., name the key conditions used in
Theorem~\ref{thm:global}, such as termination/strong-normalization, local
confluence on the skeleton, and any well-foundedness or compatibility
assumptions) so readers see the concrete requirements at a glance.
Rewrote the abstract and intro.