Conversation
Problem: the public repository had no installable Didimlog implementation. Change: add the reviewed v0.0.1 CLI, tests, package metadata, CI, release workflow, and verification artifacts. Rejected: Copy private repository history | it contains unrelated private project material. Confidence: high Scope-risk: moderate
|
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:
📝 Walkthrough<hidden_range_assignment> 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Problem: Linux CI created a shallow temporary path and the retrieval judge indexed a nonexistent fourth parent. Change: pass the sandbox root explicitly through every runtime and test call, with a regression test for path sanitization. Rejected: Pad the test path | it would preserve the production helper's hidden directory-depth dependency. Confidence: high Scope-risk: narrow
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
src/didimlog/cli.py-419-420 (1)
419-420: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle a non-integer
SystemExit.code.
SystemExit.codecan be a string whenargparseexits with a message.int(exit_signal.code or 0)then raisesValueErrorinside the exception handler and escapesmain.DidimArgumentParser.errorremoves the common path, butparser.exit(status, message)remains reachable inargparseinternals.🐛 Proposed fix
except SystemExit as exit_signal: - return int(exit_signal.code or 0) + code = exit_signal.code + if code is None: + return 0 + if isinstance(code, int): + return code + print(code, file=sys.stderr) + return EXIT_USAGE🤖 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 `@src/didimlog/cli.py` around lines 419 - 420, Update the SystemExit handling in main to safely handle non-integer exit_signal.code values, including string messages from argparse, without raising ValueError. Preserve numeric status codes and return the existing default success code when the exit code is absent, while mapping non-integer values to an appropriate nonzero status.tests/fixtures/record_cases.json-1-3 (1)
1-3: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale
tests/fixtures/record_cases.jsonexpectations or wire them through the current API.
tests/didimlog_tests/project/test_record.pyonly readsobs_ascii_success,exp_contradicts_none_success, andevd_local_success. The remaining cases are unexecuted and describe inputs the tested surface cannot exercise: cases likeobs_bad_id_formatorobs_id_date_mismatchpass--id,--output,--workspace, anddidimCLI args throughserialize_fixture_case()instead of CLI parsing, and error tokens used as case expectations do not appear incapture.py,record.py, orartifacts.py. Update the fixture description todidimif any CLI matrix remains, then either delete unused cases or add a runner that executes them.🤖 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 `@tests/fixtures/record_cases.json` around lines 1 - 3, Remove the unexecuted cases from tests/fixtures/record_cases.json, retaining only cases consumed by test_record.py, or add a runner that exercises every case through the current API; do not pass CLI flags through serialize_fixture_case(). If any CLI matrix remains, update the fixture description to refer to didim.src/didimlog/project/scaffold.py-52-59 (1)
52-59: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
PATH_ESCAPEis unreachable for a symlinked workspace.
_lstatdoes not follow symlinks. For a symlink,st_modereportsS_IFLNKand neverS_IFDIR. Line 55 therefore raisesSCAFFOLD_WORKSPACE_INVALIDfirst, and theS_ISLNKbranch on line 57 never runs. Check the symlink condition before the directory condition to emit the intended token.🐛 Proposed fix for the check order
candidate = Path(workspace) metadata = _lstat(candidate) - if metadata is None or not stat.S_ISDIR(metadata.st_mode): - raise _policy_error("SCAFFOLD_WORKSPACE_INVALID", candidate) - if stat.S_ISLNK(metadata.st_mode): + if metadata is not None and stat.S_ISLNK(metadata.st_mode): raise _policy_error("PATH_ESCAPE", candidate) + if metadata is None or not stat.S_ISDIR(metadata.st_mode): + raise _policy_error("SCAFFOLD_WORKSPACE_INVALID", candidate) return Path(os.path.abspath(candidate))🤖 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 `@src/didimlog/project/scaffold.py` around lines 52 - 59, Update _require_workspace so the stat.S_ISLNK(metadata.st_mode) check occurs before the stat.S_ISDIR directory validation, ensuring symlinked workspaces raise _policy_error("PATH_ESCAPE", candidate) instead of SCAFFOLD_WORKSPACE_INVALID; preserve the existing behavior for missing and non-directory paths.src/didimlog/claude/transaction.py-160-200 (1)
160-200: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReorder the optional type unions and separate the two methods.
Ruff reports RUF036 on lines 160, 170, and 200. Place
Noneat the end of each union. Line 199 also starts@staticmethodimmediately after theraiseon line 198 with no blank line.♻️ Proposed fix
- def _installed_digest(cls, target: dict[str, object]) -> str | None | bool: + def _installed_digest(cls, target: dict[str, object]) -> str | bool | None:- def _digest_at(parent_descriptor: int, name: str) -> str | None | bool: + def _digest_at(parent_descriptor: int, name: str) -> str | bool | None:raise FileExistsError("could not allocate rollback temporary file") + `@staticmethod` - def _current_digest(path: Path) -> str | None | bool: + def _current_digest(path: Path) -> str | bool | None:🤖 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 `@src/didimlog/claude/transaction.py` around lines 160 - 200, Update the return annotations of _installed_digest and _digest_at so None appears last in each optional union, satisfying Ruff RUF036. Add a blank line between _create_temporary and the following _current_digest declaration so the decorator is separated from the preceding method body.Source: Linters/SAST tools
src/didimlog/claude/resources.py-170-197 (1)
170-197: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the return type mismatch.
The annotation declares
tuple[Path, Path]. Line 197 builds the tuple from a generator, which types astuple[Path, ...]. A static type checker rejects that assignment. The runtime value is correct.🐛 Proposed fix
managed_path = config_path / "didimlog" - return tuple(managed_path / name for name in _RESOURCE_NAMES) + usage_name, rules_name = _RESOURCE_NAMES + return managed_path / usage_name, managed_path / rules_name🤖 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 `@src/didimlog/claude/resources.py` around lines 170 - 197, Update the return expression in materialize_resources to construct a fixed two-element tuple matching the declared tuple[Path, Path] return type, rather than using a generator-based tuple conversion. Preserve the existing resource-name ordering and paths..github/workflows/ci.yml-22-22 (1)
22-22: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDisable persisted checkout credentials.
Later steps execute repository-controlled code. Set
persist-credentials: falseonactions/checkoutso the runner does not retainGITHUB_TOKENin the local Git configuration.Proposed fix
- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + persist-credentials: false🤖 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 @.github/workflows/ci.yml at line 22, Update the actions/checkout@v4 step in the CI workflow to set persist-credentials to false, ensuring checkout does not retain GITHUB_TOKEN credentials in the local Git configuration.Source: Linters/SAST tools
src/didimlog/resources/project/README.md-1-7 (1)
1-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winA project-specific name,
improver, is baked into packaged resources. Both files are shipped inside the wheel and scaffolded into arbitrary user projects, so every user receives documents that name a foreign project. The shared root cause is a template that was copied from the originating repository without parameterizing the project identifier.
src/didimlog/resources/project/README.md#L1-L7: replaceimproveron lines 1 and 3 with a neutral term, or substitute the real project name during scaffolding.src/didimlog/resources/project/record.schema.json#L3-L6: change the$idfromhttps://improver.local/knowledge/schema/record.schema.jsonto a project-neutral URI, and rename thekh-newandkh-indexreferences in the$commentvalues on lines 6, 94, 123, and 130 todidim addanddidim index.🤖 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 `@src/didimlog/resources/project/README.md` around lines 1 - 7, Remove the originating project name from the packaged templates: in src/didimlog/resources/project/README.md lines 1-7, replace “improver” with a neutral term or substitute the actual project name during scaffolding; in src/didimlog/resources/project/record.schema.json lines 3-6, use a project-neutral $id URI and rename every kh-new and kh-index reference in $comment values at lines 6, 94, 123, and 130 to didim add and didim index.tools/didim-retrieval-ab-170-172 (1)
170-172: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCreate the credential metadata file with the restrictive mode.
write_textcreates.claude.jsonusing the process umask, thenos.chmodnarrows it to0600. Between those two calls the file holdingoauthAccountanduserIDis readable by other local users. Line 227 ininstallrepeats the pattern. Create the file with the mode already applied.🔒 Proposed fix
target = config_dir / ".claude.json" - target.write_text(json.dumps(seeded, indent=2) + "\n", encoding="utf-8") - os.chmod(target, 0o600) + payload = (json.dumps(seeded, indent=2) + "\n").encode("utf-8") + descriptor = os.open(target, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(descriptor, "wb") as stream: + stream.write(payload)🤖 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 `@tools/didim-retrieval-ab` around lines 170 - 172, Update the credential metadata creation around target.write_text and the install function’s corresponding .claude.json creation so the file is created with mode 0600 from the outset, rather than writing it first and applying os.chmod afterward. Preserve the existing JSON contents and encoding while eliminating the readable intermediate state.src/didimlog/resources/project/README.md-17-23 (1)
17-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd or remove the non-existent tutorial entry point.
src/didimlog/resources/project/README.mdtells first readers to openknowledge-harness-tutorial.html, but the file is not present and is not covered by the release allowlist. Add the file/template to the package, update the sdist allowlist if needed, or remove these references so the documented first-reader path does not point to a missing resource.🤖 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 `@src/didimlog/resources/project/README.md` around lines 17 - 23, Update the README references to knowledge-harness-tutorial.html and the project packaging configuration together: either add the tutorial file/template and include it in the release allowlist, or remove the tutorial references so the documented first-reader path only points to resources shipped in the package.src/didimlog/personal/rules_document.py-92-103 (1)
92-103: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMap the remaining
os.linkfailures toRulesDocumentError.Line 103 re-raises any non-
EEXISTOSErrorunchanged.os.linkfails withEPERMorEOPNOTSUPPon filesystems that do not support hard links, which includes some network and removable-media mounts. The caller then receives a bareOSErrorinstead of the module'sRulesDocumentErrorcontract._apply_personalinsrc/didimlog/claude/setup.pycallscreate_user_rules(plan.home)inside its setup transaction and surfaces the raw error.🛡️ Proposed fix
except OSError as exc: if exc.errno == errno.EEXIST: raise RulesConcurrentModification( "MY-RULES.md appeared during creation" ) from exc - raise + raise RulesDocumentError( + "MY-RULES.md could not be created" + ) from exc🤖 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 `@src/didimlog/personal/rules_document.py` around lines 92 - 103, Update the os.link error handling in the rules-document creation flow to convert every non-EEXIST OSError into RulesDocumentError, while preserving RulesConcurrentModification for EEXIST failures. Ensure callers such as create_user_rules used by _apply_personal receive only the module’s documented error type.
🤖 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 `@src/didimlog/claude/connect.py`:
- Around line 64-81: Update _migrate_legacy_claude in
src/didimlog/claude/connect.py: after removing the legacy block, pass the
resulting content to plan_claude_md(config) so any existing Didimlog block is
replaced rather than duplicated. Add a regression case in
tests/didimlog_tests/claude/test_connect.py covering both marker sets and assert
the applied CLAUDE.md contains exactly one DIDIMLOG:START marker.
In `@src/didimlog/claude/transaction.py`:
- Around line 72-98: Update rollback and _rollback_target so failures while
processing one target, including exceptions from backup Path.read_bytes(), are
caught and converted to False. Ensure rollback continues iterating over every
target after an individual failure, while preserving existing successful
restoration and explicit False outcomes.
In `@src/didimlog/indexing.py`:
- Around line 19-31: The index status contract must expose machine-readable
tokens separately from display text. In src/didimlog/indexing.py lines 19-31,
update IndexResult with personal_token and project_token fields, populate them
with the bare token constants, and use PROJECT_NOT_CONFIGURED for unconfigured
projects while preserving rendered labels. In src/didimlog/cli.py lines 368-377,
update the index check to compare result.personal_token and result.project_token
directly with the token constants instead of parsing display strings with
endswith or substring matching.
In `@src/didimlog/personal/index.py`:
- Around line 407-479: Wrap the full write-and-cleanup operation in write_all
with acquire_directory_lock for the destination index directory, including
temporary preparation, os.replace calls, validation, and stale .md removal.
Reuse the existing lock primitive and ensure the lock is held until all writes
and cleanup finish, while preserving current error handling and return behavior.
In `@src/didimlog/personal/render.py`:
- Around line 211-216: The vendored Mermaid bundle must be verified and
script-safe before injection. In src/didimlog/personal/render.py lines 211-216,
update _load_mermaid to read mermaid.min.js as bytes, compare its hashlib.sha256
digest with the shipped mermaid.min.js.sha256 format, raise ValueError on
mismatch, catch ModuleNotFoundError alongside existing exceptions, and
neutralize </script by replacing it with <\/script> before returning text. In
src/didimlog/personal/render.py lines 335-347, keep the {mermaid} substitution
unchanged; no direct change is needed there.
- Around line 135-144: Update _reject_raw_html to detect and reject block-level
raw HTML, including bare tags such as <div> and tags with markdown attributes,
before Markdown rendering; retain the existing inline-HTML checks and ensure
_inline_images cannot pass such content through to the extra or md_in_html
rendering path.
In `@src/didimlog/personal/secret_scan.py`:
- Around line 44-51: Update the git function’s subprocess.run call to use the
project’s bounded timeout and disable terminal prompts, while preserving the
existing argv-list execution and error propagation so TimeoutExpired continues
to be handled by main.
In `@src/didimlog/project/artifacts.py`:
- Around line 78-91: The artifact path contract is duplicated and inconsistent
between schema validation and tree loading. In src/didimlog/project/artifacts.py
lines 78-91, make the artifacts/ prefix the single required contract and enforce
it during validate_frontmatter so paths such as knowledge/raw/log.txt are
rejected consistently. In src/didimlog/project/record.py lines 232-248, remove
_validate_artifact_path and reuse artifacts.check_artifact_path_format,
including its control-character and UTF-8 encodability checks.
In `@src/didimlog/project/capture.py`:
- Around line 335-357: Update the file-writing flow around the O_EXCL creation
and write loop to unlink the newly created record when writing or fsyncing
fails, while still closing the file descriptor. Reuse the identity-safe cleanup
approach from scaffold._create_file so only the intended partial file is
removed, then preserve the existing directory-descriptor cleanup and propagate
the original failure.
In `@tests/didimlog_tests/project/test_artifacts.py`:
- Around line 380-395: Update _seed_repository to return its isolated Git
environment, then update every call site to unpack the fourth returned value. In
the second add/commit sequence, reuse that returned environment instead of
rebuilding it from os.environ, while preserving the test-specific author and
committer identity settings.
In `@tests/didimlog_tests/test_package_lifecycle.py`:
- Around line 68-91: In the lifecycle test setup, build the isolated environment
before the repository initialization commands, then pass it via env= to every
subprocess.run git call, including git init and both git config invocations.
Preserve the existing isolation values for GIT_CONFIG_GLOBAL,
GIT_CONFIG_NOSYSTEM, HOME, and related variables.
In `@tests/didimlog_tests/test_release.py`:
- Around line 71-81: Replace raw substring assertions in
test_release_uses_published_immutable_release_oidc_and_protected_environment
with YAML parsing and structural assertions. Handle PyYAML’s bare on key as
either True or "on", require only the published release trigger, and validate
each job’s permissions and absence of passwords; retain explicit checks for the
publish job’s pypi environment and id-token write permission.
In `@tools/didim-retrieval-ab`:
- Around line 342-350: Update the assistant-event handling in parse to use
guarded .get() access for message, tool-use name, and tool-use input instead of
direct indexing. Preserve collecting valid tool-use blocks while skipping
incomplete assistant events or blocks without required fields, so malformed
events do not abort the trial.
- Around line 300-325: Update the session function’s subprocess.run call to use
a bounded timeout, and handle subprocess.TimeoutExpired by recording the session
as an error with the timeout details and returning the parsed session result.
Preserve writing available output to log_path and include a nonzero return
status so stalled Claude sessions cannot block the trial harness indefinitely.
- Around line 130-148: Update grant_keychain so the Claude credential is never
passed through subprocess argv: remove the -w/secret command-line arguments and
use Apple Security frameworks or an existing secure wrapper that supports
non-interactive credential input. Preserve the existing service and account
values and return behavior.
---
Minor comments:
In @.github/workflows/ci.yml:
- Line 22: Update the actions/checkout@v4 step in the CI workflow to set
persist-credentials to false, ensuring checkout does not retain GITHUB_TOKEN
credentials in the local Git configuration.
In `@src/didimlog/claude/resources.py`:
- Around line 170-197: Update the return expression in materialize_resources to
construct a fixed two-element tuple matching the declared tuple[Path, Path]
return type, rather than using a generator-based tuple conversion. Preserve the
existing resource-name ordering and paths.
In `@src/didimlog/claude/transaction.py`:
- Around line 160-200: Update the return annotations of _installed_digest and
_digest_at so None appears last in each optional union, satisfying Ruff RUF036.
Add a blank line between _create_temporary and the following _current_digest
declaration so the decorator is separated from the preceding method body.
In `@src/didimlog/cli.py`:
- Around line 419-420: Update the SystemExit handling in main to safely handle
non-integer exit_signal.code values, including string messages from argparse,
without raising ValueError. Preserve numeric status codes and return the
existing default success code when the exit code is absent, while mapping
non-integer values to an appropriate nonzero status.
In `@src/didimlog/personal/rules_document.py`:
- Around line 92-103: Update the os.link error handling in the rules-document
creation flow to convert every non-EEXIST OSError into RulesDocumentError, while
preserving RulesConcurrentModification for EEXIST failures. Ensure callers such
as create_user_rules used by _apply_personal receive only the module’s
documented error type.
In `@src/didimlog/project/scaffold.py`:
- Around line 52-59: Update _require_workspace so the
stat.S_ISLNK(metadata.st_mode) check occurs before the stat.S_ISDIR directory
validation, ensuring symlinked workspaces raise _policy_error("PATH_ESCAPE",
candidate) instead of SCAFFOLD_WORKSPACE_INVALID; preserve the existing behavior
for missing and non-directory paths.
In `@src/didimlog/resources/project/README.md`:
- Around line 1-7: Remove the originating project name from the packaged
templates: in src/didimlog/resources/project/README.md lines 1-7, replace
“improver” with a neutral term or substitute the actual project name during
scaffolding; in src/didimlog/resources/project/record.schema.json lines 3-6, use
a project-neutral $id URI and rename every kh-new and kh-index reference in
$comment values at lines 6, 94, 123, and 130 to didim add and didim index.
- Around line 17-23: Update the README references to
knowledge-harness-tutorial.html and the project packaging configuration
together: either add the tutorial file/template and include it in the release
allowlist, or remove the tutorial references so the documented first-reader path
only points to resources shipped in the package.
In `@tests/fixtures/record_cases.json`:
- Around line 1-3: Remove the unexecuted cases from
tests/fixtures/record_cases.json, retaining only cases consumed by
test_record.py, or add a runner that exercises every case through the current
API; do not pass CLI flags through serialize_fixture_case(). If any CLI matrix
remains, update the fixture description to refer to didim.
In `@tools/didim-retrieval-ab`:
- Around line 170-172: Update the credential metadata creation around
target.write_text and the install function’s corresponding .claude.json creation
so the file is created with mode 0600 from the outset, rather than writing it
first and applying os.chmod afterward. Preserve the existing JSON contents and
encoding while eliminating the readable intermediate state.
---
Nitpick comments:
In `@src/didimlog/claude/connect.py`:
- Around line 236-267: Add transaction-backup retention by deleting backup files
created by _backup_original after a successful transaction commit, or otherwise
limiting them to the most recent transaction. Track the returned backup paths
through the commit flow and remove them only after commit succeeds, while
preserving backups needed for rollback on failed transactions.
In `@src/didimlog/claude/probe.py`:
- Around line 10-20: Promote the helpers consumed by the probe module to stable
public interfaces: replace private imports and references to _discover_git_root,
_personal_check, _prepared_project, _project_check, _packaged_resources,
_read_optional, _load_settings, _is_managed_session_start_hook, and
_SESSION_START_SUFFIX with public exports, or centralize them in a dedicated
internal API module. Update probe.py and the defining modules consistently so
consumers no longer depend on underscore-prefixed symbols.
- Around line 90-91: Extract the repeated home normalization into a shared
helper named _selected_home(home), preserving the existing Path.home/Path(home)
selection and absolute-path conversion. Update inspect in
src/didimlog/claude/probe.py at lines 90-91, status_text and doctor_text in
src/didimlog/claude/status.py at lines 109-110 and 161-162, and _plan_personal
in src/didimlog/claude/setup.py at lines 75-76 to call the helper instead of
duplicating the two-line logic.
In `@src/didimlog/claude/resources.py`:
- Around line 98-115: Update the resource-reading function containing the
os.open call to initialize descriptor as an explicit int-or-None sentinel before
the try block, and change the finally cleanup to close it only when it is not
None. Preserve the existing safe-read and error-handling behavior.
In `@src/didimlog/claude/setup.py`:
- Around line 302-319: Update the _postcheck(plan) exception handler so failures
from journal.rollback() do not replace the original post-check exception; catch
any rollback exception and attach it as context to the original failure, then
re-raise the original exception.
- Around line 112-117: Replace the substring-based check in the index_current
calculation with a boolean structural flag set when the personal-index directory
creation condition is handled while building changes. Use that flag in
index_current instead of searching change message text, and preserve the
existing behavior of appending the personal index update when the index is not
current.
- Around line 176-184: Update the launcher-missing branch in the setup flow
around _find_launcher() to raise DidimError instead of ValueError, supplying the
module’s established token, exit code, and a repair hint for didim being absent
from PATH. Leave the successful plan_connect flow unchanged.
In `@src/didimlog/claude/status.py`:
- Around line 99-104: Update the Ruff configuration in pyproject.toml to add
scoped per-file ignores for S105 and S106 on src/didimlog/claude/status.py and
src/didimlog/claude/probe.py, covering diagnostic token fields without adding
inline suppressions.
In `@src/didimlog/claude/transaction.py`:
- Around line 23-29: Update __init__ when loading existing journal data to catch
JSONDecodeError and validate the parsed value is a mapping containing a version
and a dictionary-valued targets field; raise a clear ValueError for malformed
JSON or invalid structure before assigning it to self.data, while preserving the
default data and reset behavior.
- Around line 216-236: Update _save to open the containing directory after
os.replace completes and fsync its file descriptor, matching
config.py::write_if_unchanged’s durability behavior; ensure the directory
descriptor is closed reliably while preserving the existing temporary-file
cleanup.
In `@src/didimlog/cli.py`:
- Around line 282-286: Align _disconnect_claude with _connect_claude by
requiring --yes or an interactive confirmation before applying changes, while
preserving plan preview output. Register the --yes flag on the disconnect claude
subparser in build_parser and pass the approval through the existing disconnect
flow before invoking _apply_claude.
In `@src/didimlog/indexing.py`:
- Around line 43-58: Update the subprocess call in _discover_git_root to read
git’s stdout as bytes by removing text=True, then decode the stripped path using
the filesystem encoding before the lstat validation. Preserve the existing
timeout, fixed arguments, return-code handling, and failure behavior.
- Around line 177-187: Update run_index around personal_index.write_all and
project_index.write_index to acquire and hold the repository’s existing
acquire_directory_lock for the complete index-write sequence, including both
indexes. Ensure the lock also covers the configured-project branch while
preserving the existing status results and return behavior; use the established
locking boundary rather than adding a separate locking mechanism.
In `@src/didimlog/locking.py`:
- Around line 28-43: Update the locking flow around fcntl.flock in the
lock-acquisition function to use LOCK_EX | LOCK_NB, retry until a defined
deadline, and raise a clear timeout error naming _LOCK_NAME when acquisition
does not succeed. Preserve the existing descriptor validation and return
behavior after the lock is acquired.
In `@src/didimlog/personal/index.py`:
- Around line 134-143: Update _validate_nonrecursive_layout to catch OSError
raised by root.iterdir() and re-raise it as KnowledgeIndexError, preserving the
original exception context and the module’s established error behavior.
- Around line 205-233: Update _markdown_files to resolve project_dir once before
iterating over Markdown files, store the resulting project root, and reuse it
for each path containment check; preserve the existing error handling and
validation behavior.
In `@src/didimlog/personal/lesson_writing.py`:
- Around line 122-131: Initialize the descriptor variable to a safe sentinel
before the try block, then update the OSError cleanup in the lesson directory
validation flow to close it only when a valid descriptor was opened; remove the
locals() check while preserving the existing LessonInvalid propagation.
In `@src/didimlog/personal/paths.py`:
- Around line 124-132: Update the git subprocess invocation in the surrounding
path-resolution function to resolve the executable with shutil.which("git")
before calling subprocess.run. If no executable is found, raise or return the
existing GIT_UNAVAILABLE outcome, and pass the resolved path as the command
executable while preserving the fixed arguments and cwd behavior.
In `@src/didimlog/personal/render.py`:
- Around line 238-267: Update _replace_output to call the existing
_sync_directory helper on output.parent after a successful os.replace, matching
the publish flow in rules_document.py. Wrap os.replace failures caused by the
destination becoming a directory and translate them to the module’s ValueError
contract, while preserving temporary-file cleanup and propagating unrelated
errors.
- Around line 17-20: Update _reject_raw_html to remove reliance on internal
symbols from markdown.inlinepatterns,
markdown.preprocessors.NormalizeWhitespace, and FencedBlockPreprocessor. Replace
them with documented Markdown APIs or locally defined equivalent patterns and
processing logic, preserving the existing raw-HTML rejection behavior across
Markdown upgrades.
In `@src/didimlog/personal/rules_document.py`:
- Line 63: Annotate the home parameter in create_user_rules with its accepted
path-like or string type, using the existing future annotations support and
matching the project’s established convention for optional path inputs.
In `@src/didimlog/personal/secret_scan.py`:
- Around line 63-83: Bound the data loaded by staged_blob by streaming git show
output and retaining only a bounded prefix sufficient for the detectors, such as
the first 8192 bytes inspected by is_binary. Preserve the existing stage-zero
path handling and error behavior, unless full-file scanning is explicitly
required by scan_bytes.
In `@src/didimlog/project/capture.py`:
- Around line 360-366: Update _refresh_index to capture the caught exception and
print its details to stderr on a second line after preserving the exact
PROJECT_INDEX_STALE: run didim index token line. Update the full-stderr
assertion in test_capture.py to expect both lines.
- Around line 20-41: Update record.py to expose the four cross-module helpers
under public names, then change capture.py to import and use those public
helpers instead of private names. Replace the local _PREFIX_BY_TYPE definition
with record.PREFIX_BY_TYPE, and remove the local _canonicalize_tag
implementation so capture() reuses record.py’s canonicalization and validation
logic.
In `@src/didimlog/project/record.py`:
- Around line 512-521: Harden write_atomic to match capture.py’s
_write_create_only: open with O_NOFOLLOW and O_CLOEXEC in addition to the
existing create-only flags, then fsync the parent directory after the file is
durably written. Preserve the existing ALREADY_EXISTS handling and avoid
duplicating writer logic if both call sites can safely share one implementation.
- Around line 232-248: Replace the duplicated validation logic in
_validate_artifact_path with the shared check_artifact_path_format helper from
the artifacts module, while preserving the existing SchemaError and evidence_id
handling. Ensure validate_frontmatter uses the same checks as capture-time
validation, including rejection of non-encodable characters and 0x7F.
In `@src/didimlog/resources/personal/LESSON_WRITING_RULES.md`:
- Line 23: LESSON_WRITING_RULES.md의 23행 문장을 문서에서 사용하는 “-한다” 종결형으로 통일한다. `title`,
`topic`, `tags`의 기존 제약 내용은 그대로 유지한다.
In `@tests/didimlog_tests/claude/test_connect.py`:
- Around line 131-148: Extend the fresh-connect assertions in the test around
apply_connect to verify that each newly created file, including settings.json,
CLAUDE.md, and packaged resource files, has permission mode 0o600. Use the
existing file paths and stat-based mode inspection without changing the current
content assertions.
- Around line 208-222: Extend the test around _migrate_legacy_claude to include
an existing DIDIMLOG:START/END block alongside the legacy block in CLAUDE.md,
then assert migration preserves a single correctly updated Didimlog block
without inserting a duplicate. Keep the existing user-prefix and user-owned
marker coverage intact.
In `@tests/didimlog_tests/claude/test_hook.py`:
- Around line 34-50: Resolve the personal knowledge root through data_home
instead of hardcoding "knowledge": import and use data_home in
tests/didimlog_tests/claude/test_hook.py at lines 34-50 and 111, and replace the
corresponding self.home / "knowledge" / "index" expressions in
tests/didimlog_tests/claude/test_status.py at lines 122 and 87 with
data_home(self.home) / "index".
In `@tests/didimlog_tests/claude/test_paths.py`:
- Around line 25-48: Add a test alongside
test_environment_config_takes_priority_over_default and
test_default_config_is_dot_claude_inside_home that calls config_dir with
CLAUDE_CONFIG_DIR set to an empty string, then asserts it returns home /
".claude" resolved. Use the existing temporary home setup and preserve the
documented fallback behavior.
In `@tests/didimlog_tests/claude/test_resources.py`:
- Around line 49-110: Add three tests alongside the existing materialization
tests: create a symlink for the config directory and assert materialization
raises ValueError via _open_directory; create a symlink for the didimlog
directory and assert ValueError via _open_managed_directory; and create a
symlink for a managed resource file and assert ValueError via
_read_regular_file. Use temporary directories and skip or guard appropriately
where symlink creation is unsupported.
In `@tests/didimlog_tests/claude/test_setup_apply.py`:
- Around line 159-168: Update test_failed_postcheck_is_not_reported_as_success
to snapshot self.config before apply_setup, then assert self.config matches that
snapshot after the expected DidimError, while preserving the existing
SETUP_POSTCHECK_FAILED token assertion to verify journal.rollback restored the
Claude surface.
In `@tests/didimlog_tests/claude/test_setup_plan.py`:
- Around line 93-110: Add a test covering the missing-launcher branch in
plan_setup: patch didimlog.claude.setup._find_launcher to return None, invoke
setup with Claude enabled, and assert that the expected error is raised. Keep
the existing skip-without-probe test unchanged.
- Line 76: Update the version assertion in the plan setup test to compare
plan.version with the didimlog.version() API result instead of hardcoding
"0.0.1". Leave the fixed released-version assertion to the dedicated
package-metadata test.
In `@tests/didimlog_tests/personal/test_book_state.py`:
- Around line 50-58: Register self.temporary_directory.cleanup with addCleanup
immediately after creating the TemporaryDirectory in setUp, then remove tearDown
in tests/didimlog_tests/personal/test_book_state.py (lines 50-58) and
tests/didimlog_tests/personal/test_render.py (lines 29-40), preserving the
existing directory setup.
In `@tests/didimlog_tests/personal/test_index.py`:
- Line 213: Remove the tautological assertEqual around
z_output.encode("utf-8").decode("utf-8") in the affected test; rely on the
existing exact UTF-8 byte assertion later in the test, and do not replace it
unless the test explicitly needs to validate rejection of lone surrogates.
In `@tests/didimlog_tests/personal/test_lesson_writing.py`:
- Around line 327-343: Add a focused test for lesson_writing.main that supplies
lesson input through stdin and verifies the user-visible exit-code mapping:
LessonSecret→5, LessonInvalid→2, LessonExists→3, and LessonError→4. Reuse the
existing test fixtures and invocation helpers where available; otherwise confirm
an existing CLI test already covers all four paths.
In `@tests/didimlog_tests/personal/test_secret_scan.py`:
- Line 32: Update the test class setup around setUp and the git init command to
detect git availability with shutil.which and skip the suite when it is absent,
before invoking subprocess.run. Preserve the existing git initialization
behavior when git is installed.
- Around line 76-145: Add a test near the existing scan status tests, using the
module’s subprocess entry point to run from a directory outside any Git
repository. Ensure the temporary directory is not under a Git worktree, then
assert the process returns status 2 and stderr contains SECRET_SCAN_ERROR,
covering the staged_paths() failure path.
- Around line 37-45: Update the test helper method run_command to pass a finite
timeout to subprocess.run, using a suitable test-level default so hung git or
scanner processes fail within the named test rather than blocking indefinitely.
Preserve the existing command arguments, environment, output capture, and check
behavior.
In `@tests/didimlog_tests/project/test_capture.py`:
- Around line 432-433: Update the retry-related tests around capture and
collide_once to patch os.open directly via its module path, replacing
mock.patch.object(record_module.os, ...) with mock.patch("os.open", ...). Apply
the same change to the additional occurrence, while leaving capture and the
retry assertions unchanged.
- Around line 494-536: Update
test_two_processes_receive_distinct_ids_and_preserve_both_bodies to drain
results from the results queue before joining the child processes. Collect both
outcomes immediately after start.set(), then perform the existing join,
termination, and exit-code assertions using the already collected results.
In `@tests/didimlog_tests/test_cli_commands.py`:
- Line 142: In the test invoking the CLI setup command, rename the unused
unpacked variable stdout to _stdout while preserving code and stderr handling.
- Around line 106-130: Strengthen
test_setup_dry_run_and_yes_use_the_same_plan_summary by asserting that
yes_stdout begins with the complete dry_stdout summary, accounting for the
additional completion line emitted by --yes. Keep the existing content and
exit-code assertions, and use a prefix comparison rather than only checking
individual change strings.
- Around line 377-380: Replace the hardcoded expected version in the CLI version
test with the version imported from the didimlog package, while preserving the
existing newline and command output assertion. Keep the current fixed subprocess
invocations unchanged; the S603 findings do not require modifications.
In `@tests/didimlog_tests/test_cli.py`:
- Around line 72-102: Add CLI-boundary coverage for _as_didim_error by mocking
publish_lesson and raising LessonExists, LessonInvalid, and another exception.
Assert each case produces the expected token and exit code—LESSON_EXISTS with
EXIT_POLICY, LESSON_INVALID with EXIT_USAGE, and COMMAND_FAILED for the generic
exception—while preserving the specific-lesson branch order.</code>
In `@tests/didimlog_tests/test_indexing.py`:
- Around line 134-157: Add tests alongside the existing indexing status tests
for the uncovered branches: verify a project index containing INDEX.md plus
another file returns PROJECT_INDEX_EXTRA, verify invalid source data in the
project tree returns PROJECT_INDEX_INVALID_SOURCE, and replace the personal
index directory with a symlink to confirm the symlink case returns
PERSONAL_INDEX_EXTRA without following it outside the knowledge root.
- Around line 34-46: Update _git_project and the corresponding git
initialization at lines 88-93 to invoke git with an explicit sanitized
environment, excluding ambient repository settings such as GIT_DIR and global
init configuration while preserving the test's required environment. Ensure both
repository setups use this isolated environment so _discover_git_root behaves
deterministically.
In `@tests/didimlog_tests/test_locking.py`:
- Around line 38-43: Update the symlink-rejection assertion around
acquire_directory_lock to capture the raised OSError and verify its errno is
ELOOP or EMLINK, preserving the existing cleanup and target-content assertions.
In `@tests/didimlog_tests/test_package_lifecycle.py`:
- Line 52: Update the wheel glob in the test flow to interpolate the existing
VERSION symbol instead of hardcoding 0.0.1, applying the same change to the
analogous globs near the other referenced locations. Replace bare next(...) with
an assertion-friendly lookup that reports clearly when no matching wheel is
produced.
In `@tests/didimlog_tests/test_package_metadata.py`:
- Line 20: Update the project dependency assertion in test_package_metadata.py
and the corresponding assertion in test_release.py to expect the compatible
Markdown range (Markdown>=3.10.2,<4) instead of the exact 3.10.2 pin, while
leaving reproducibility to uv.lock.
In `@tests/didimlog_tests/test_release.py`:
- Around line 83-94: Update test_wheel_and_sdist_follow_the_public_allowlist to
resolve uv with shutil.which and skip the test via skipTest when it is
unavailable, matching test_package_lifecycle.py. Use the resolved executable in
subprocess.run, and replace bare next calls for wheel and sdist discovery with
assertions or explicit failures that report when the build produces no artifact.
In `@tests/didimlog_tests/test_retrieval_ab.py`:
- Around line 279-293: Add mutations for control.followed_sentinel,
treatment.followed_sentinel, and forced.followed_sentinel in
test_session_error_missing_index_decoy_or_excess_reads_invalidates_ab. Set each
to the invalid value used by failure_reasons and retain the existing subTest
loop so all three branches are exercised.
In `@tools/didim-retrieval-ab`:
- Around line 291-298: Update make_git_project so every git
init/config/add/commit command runs with an environment isolated from host and
system Git configuration, disabling inherited templates, hooks, signing, and
other user-specific settings while preserving the fixture’s existing repository
setup.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c84e4720-c2e6-4406-bacd-665a6dd94597
⛔ Files ignored due to path filters (2)
src/didimlog/resources/personal/mermaid.min.jsis excluded by!**/*.min.jsuv.lockis excluded by!**/*.lock
📒 Files selected for processing (98)
.github/workflows/ci.yml.github/workflows/release.ymlCHANGELOG.mdTHIRD_PARTY_NOTICES.mdartifacts/didimlog-v0.0.1-package-lifecycle.jsonartifacts/didimlog-v0.0.1-retrieval-ab.jsonpyproject.tomlsrc/didimlog/__init__.pysrc/didimlog/claude/__init__.pysrc/didimlog/claude/config.pysrc/didimlog/claude/connect.pysrc/didimlog/claude/hook.pysrc/didimlog/claude/paths.pysrc/didimlog/claude/probe.pysrc/didimlog/claude/resources.pysrc/didimlog/claude/setup.pysrc/didimlog/claude/status.pysrc/didimlog/claude/transaction.pysrc/didimlog/cli.pysrc/didimlog/errors.pysrc/didimlog/indexing.pysrc/didimlog/locking.pysrc/didimlog/personal/__init__.pysrc/didimlog/personal/book_state.pysrc/didimlog/personal/index.pysrc/didimlog/personal/lesson.pysrc/didimlog/personal/lesson_writing.pysrc/didimlog/personal/paths.pysrc/didimlog/personal/render.pysrc/didimlog/personal/rules_document.pysrc/didimlog/personal/secret_scan.pysrc/didimlog/project/__init__.pysrc/didimlog/project/artifacts.pysrc/didimlog/project/capture.pysrc/didimlog/project/index.pysrc/didimlog/project/record.pysrc/didimlog/project/resources.pysrc/didimlog/project/scaffold.pysrc/didimlog/project/tree.pysrc/didimlog/resources/__init__.pysrc/didimlog/resources/personal/KNOWLEDGE_USAGE.mdsrc/didimlog/resources/personal/LESSON_WRITING_RULES.mdsrc/didimlog/resources/personal/MERMAID-LICENSEsrc/didimlog/resources/personal/MERMAID-VERSIONsrc/didimlog/resources/personal/__init__.pysrc/didimlog/resources/personal/mermaid.min.js.sha256src/didimlog/resources/project/POINTER.mdsrc/didimlog/resources/project/README.mdsrc/didimlog/resources/project/__init__.pysrc/didimlog/resources/project/active-harness.mdsrc/didimlog/resources/project/record.schema.jsontests/__init__.pytests/didimlog_tests/__init__.pytests/didimlog_tests/claude/__init__.pytests/didimlog_tests/claude/test_config.pytests/didimlog_tests/claude/test_connect.pytests/didimlog_tests/claude/test_hook.pytests/didimlog_tests/claude/test_paths.pytests/didimlog_tests/claude/test_resources.pytests/didimlog_tests/claude/test_setup_apply.pytests/didimlog_tests/claude/test_setup_plan.pytests/didimlog_tests/claude/test_status.pytests/didimlog_tests/claude/test_transaction.pytests/didimlog_tests/personal/__init__.pytests/didimlog_tests/personal/fixtures/demo-api-cache.mdtests/didimlog_tests/personal/fixtures/expired-boundary.mdtests/didimlog_tests/personal/fixtures/jpa-n-plus-one.mdtests/didimlog_tests/personal/fixtures/kafka-idempotence.mdtests/didimlog_tests/personal/fixtures/korean-lesson.mdtests/didimlog_tests/personal/fixtures/malformed-no-close.mdtests/didimlog_tests/personal/fixtures/malformed-no-colon.mdtests/didimlog_tests/personal/fixtures/multiline-attempt.mdtests/didimlog_tests/personal/fixtures/other-project.mdtests/didimlog_tests/personal/test_book_state.pytests/didimlog_tests/personal/test_index.pytests/didimlog_tests/personal/test_lesson.pytests/didimlog_tests/personal/test_lesson_writing.pytests/didimlog_tests/personal/test_paths.pytests/didimlog_tests/personal/test_render.pytests/didimlog_tests/personal/test_rules_document.pytests/didimlog_tests/personal/test_secret_scan.pytests/didimlog_tests/project/__init__.pytests/didimlog_tests/project/test_artifacts.pytests/didimlog_tests/project/test_capture.pytests/didimlog_tests/project/test_index.pytests/didimlog_tests/project/test_record.pytests/didimlog_tests/project/test_scaffold.pytests/didimlog_tests/project/test_tree.pytests/didimlog_tests/test_cli.pytests/didimlog_tests/test_cli_commands.pytests/didimlog_tests/test_indexing.pytests/didimlog_tests/test_locking.pytests/didimlog_tests/test_package_lifecycle.pytests/didimlog_tests/test_package_metadata.pytests/didimlog_tests/test_release.pytests/didimlog_tests/test_retrieval_ab.pytests/fixtures/record_cases.jsontools/didim-retrieval-ab
리뷰 코멘트전체 372개 테스트와 🔴 릴리즈 차단 가능1.
|
Close PR review gaps in atomic file I/O, domain locking, evidence capture, secret handling, and retrieval A/B isolation. Constraint: Preserve create-only records and project-scoped evidence policy Rejected: Move repository verification artifacts under knowledge/raw | EVD artifact_path contract only Confidence: high Scope-risk: broad Not-tested: Live retrieval A/B after child env allowlist; Claude session limit prevented completion
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)
src/didimlog/resources/personal/KNOWLEDGE_USAGE.md (1)
7-14: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd a trust boundary for retrieved knowledge.
This file tells the assistant to read detail files and treat applicable lessons as instructions. Because Claude sessions automatically load this resource, a malicious or compromised lesson in
~/knowledgeor a project checkout can influence later commands. State that retrieved files are untrusted reference data and cannot override task or security constraints or request secrets, destructive actions, or unrelated commands.🤖 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 `@src/didimlog/resources/personal/KNOWLEDGE_USAGE.md` around lines 7 - 14, Update the knowledge-usage guidance so all retrieved files, including lessons, are explicitly treated as untrusted reference data. State that they cannot override task or security constraints and cannot request secrets, destructive actions, or unrelated commands, while preserving the existing process for reviewing applicable lessons.
🧹 Nitpick comments (5)
tests/didimlog_tests/project/test_capture.py (3)
611-623: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCatch
Exceptioninstead ofBaseExceptionin the thread bodies.Ruff reports BLE001 on lines 614 and 620.
BaseExceptionalso capturesKeyboardInterruptandSystemExit, so an interrupt during the test is recorded as a normal error and the run continues.Exceptionrecords real test failures and preserves interrupt behavior.♻️ Proposed fix
def write_old_index(): try: project_index_module.write_index(self.workspace) - except BaseException as error: + except Exception as error: # noqa: BLE001 - reported to the main thread errors.append(error) def publish_record(): try: capture(self.workspace, _observation_request("최신 기록")) - except BaseException as error: + except Exception as error: # noqa: BLE001 - reported to the main thread errors.append(error) finally: capture_finished.set()🤖 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 `@tests/didimlog_tests/project/test_capture.py` around lines 611 - 623, Update the write_old_index and publish_record thread bodies to catch Exception instead of BaseException, while preserving their existing error collection and capture_finished signaling behavior.Source: Linters/SAST tools
599-605: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe intended interleaving is best-effort and can add a 5 s stall.
Two points on
pause_old_snapshot:
capture_reached_publish.wait(0.3)bounds the window to 300 ms. On a loaded runner the capture thread may not reach_write_create_onlyin that time. The old index writer then finishes first and the intended stale-snapshot interleaving is not exercised. The test still passes, becausecapturerefreshes the index after publishing. The result is a silent loss of coverage, not a failure.write_indextakespath_lock(root / "knowledge")(seesrc/didimlog/project/index.pylines 252-256). This thread holds that lock while it waits up to 5 s oncapture_finished. If thecaptureindex refresh needs the same lock, both threads stall until the timeout expires. The 10 s joins still complete the test, but each run can cost 5 s.Consider replacing the bare timeout with an assertion that the window was reached, so lost coverage becomes visible.
🤖 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 `@tests/didimlog_tests/project/test_capture.py` around lines 599 - 605, Update pause_old_snapshot in the test interleaving so failure to reach the capture publish window is asserted rather than silently ignored. Replace the bare capture_reached_publish.wait(0.3) condition with an assertion that validates the event was reached before waiting for capture_finished, while preserving the existing synchronization sequence.
702-724: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
ModuleNotFoundErrorfallback is unreachable.Line 25 already imports
didimlog.project.indexasproject_index_moduleat module import time. If that import failed, this test file would not load. Theimportlib.import_modulecall at line 703 therefore always succeeds, andreal_index_moduleis alwaysTrue. The stub-module branch and thereal_index_moduleflag add complexity without a reachable path.Simplify by patching
project_index_module._write_index_lockeddirectly.🤖 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 `@tests/didimlog_tests/project/test_capture.py` around lines 702 - 724, Remove the redundant importlib fallback and real_index_module branching in the failure test. Reuse the module-level project_index_module import and patch its _write_index_locked attribute directly with fail_index inside the existing ExitStack.tests/didimlog_tests/project/test_record.py (1)
227-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding cases for the remaining rejection rules.
The three new cases cover parent traversal, a leading
./, and an empty segment. Theartifact_pathpattern insrc/didimlog/resources/project/record.schema.jsonalso rejects backslashes, control characters, a trailing slash, and a missing final component. Those rules currently have no test.♻️ Suggested additional cases
for artifact_path in ( "/etc/passwd", "knowledge/raw/../outside.bin", "./knowledge/raw/report.bin", "knowledge//raw/report.bin", + "knowledge/raw/", + "knowledge/raw/dir/", + "knowledge/raw/sub\\file.bin", + "knowledge/raw/rep\u0001ort.bin", ):🤖 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 `@tests/didimlog_tests/project/test_record.py` around lines 227 - 229, Add test cases alongside the existing invalid artifact paths in the relevant record validation test to cover rejection of backslashes, control characters, trailing slashes, and paths missing a final component, matching the constraints defined by the artifact_path pattern in record.schema.json.src/didimlog/locking.py (1)
13-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that the returned descriptor does not own the lock alone, and add a release helper.
os.dupcreates a new descriptor that shares one open file description withparent_descriptor.flockis attached to the open file description, not to the descriptor. The kernel releases the lock only after every descriptor of that description is closed. A caller that closes the returned descriptor but keepsparent_descriptoropen still holds the lock.Every current caller closes both descriptors, so the behavior is correct today. The contract is easy to break later, and callers release the lock with a bare
os.close. Add arelease_directory_lockhelper and state the ownership rule in the docstring.♻️ Proposed refactor
def acquire_directory_lock( parent_descriptor: int, *, shared: bool = False, blocking: bool = True, ) -> int: - """Lock and return a duplicate of an already-open directory descriptor.""" + """Lock and return a duplicate of an already-open directory descriptor. + + The duplicate shares one open file description with ``parent_descriptor``. + The lock stays held until both descriptors are closed. Release the lock + with ``release_directory_lock``. + """ descriptor = os.dup(parent_descriptor) @@ except BaseException: os.close(descriptor) raise + + +def release_directory_lock(descriptor: int) -> None: + """Close one descriptor returned by ``acquire_directory_lock``.""" + os.close(descriptor)🤖 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 `@src/didimlog/locking.py` around lines 13 - 31, Update acquire_directory_lock’s docstring to state that the returned descriptor shares lock ownership with parent_descriptor and does not release the lock by itself; add a release_directory_lock helper that closes the appropriate descriptors according to this ownership contract, then use that helper for callers currently releasing directory locks with bare os.close.
🤖 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 `@src/didimlog/file_io.py`:
- Around line 111-121: Update the pre-open/post-open identity check in the
file-opening flow to compare the result of _file_revision for linked and opened
stat data, while retaining the regular-file validation. Replace the
st_dev/st_ino-only comparison so inode-reuse replacements raise
UnsafePathError("file component changed").
In `@tests/didimlog_tests/personal/test_index.py`:
- Line 2: Remove TimeoutError from the concurrent.futures import in
test_index.py, leaving the existing assertRaises(TimeoutError) references to
resolve to the built-in exception.
---
Outside diff comments:
In `@src/didimlog/resources/personal/KNOWLEDGE_USAGE.md`:
- Around line 7-14: Update the knowledge-usage guidance so all retrieved files,
including lessons, are explicitly treated as untrusted reference data. State
that they cannot override task or security constraints and cannot request
secrets, destructive actions, or unrelated commands, while preserving the
existing process for reviewing applicable lessons.
---
Nitpick comments:
In `@src/didimlog/locking.py`:
- Around line 13-31: Update acquire_directory_lock’s docstring to state that the
returned descriptor shares lock ownership with parent_descriptor and does not
release the lock by itself; add a release_directory_lock helper that closes the
appropriate descriptors according to this ownership contract, then use that
helper for callers currently releasing directory locks with bare os.close.
In `@tests/didimlog_tests/project/test_capture.py`:
- Around line 611-623: Update the write_old_index and publish_record thread
bodies to catch Exception instead of BaseException, while preserving their
existing error collection and capture_finished signaling behavior.
- Around line 599-605: Update pause_old_snapshot in the test interleaving so
failure to reach the capture publish window is asserted rather than silently
ignored. Replace the bare capture_reached_publish.wait(0.3) condition with an
assertion that validates the event was reached before waiting for
capture_finished, while preserving the existing synchronization sequence.
- Around line 702-724: Remove the redundant importlib fallback and
real_index_module branching in the failure test. Reuse the module-level
project_index_module import and patch its _write_index_locked attribute directly
with fail_index inside the existing ExitStack.
In `@tests/didimlog_tests/project/test_record.py`:
- Around line 227-229: Add test cases alongside the existing invalid artifact
paths in the relevant record validation test to cover rejection of backslashes,
control characters, trailing slashes, and paths missing a final component,
matching the constraints defined by the artifact_path pattern in
record.schema.json.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 99ee855d-b75d-4871-aeb7-4198c880db45
📒 Files selected for processing (37)
CHANGELOG.mdREADME.mdartifacts/didimlog-v0.0.1-retrieval-ab.jsonsrc/didimlog/claude/config.pysrc/didimlog/cli.pysrc/didimlog/errors.pysrc/didimlog/file_io.pysrc/didimlog/indexing.pysrc/didimlog/locking.pysrc/didimlog/personal/book_state.pysrc/didimlog/personal/index.pysrc/didimlog/personal/lesson.pysrc/didimlog/personal/lesson_writing.pysrc/didimlog/project/artifacts.pysrc/didimlog/project/capture.pysrc/didimlog/project/index.pysrc/didimlog/project/record.pysrc/didimlog/project/scaffold.pysrc/didimlog/project/tree.pysrc/didimlog/resources/personal/KNOWLEDGE_USAGE.mdsrc/didimlog/resources/project/README.mdsrc/didimlog/resources/project/record.schema.jsontests/didimlog_tests/claude/test_config.pytests/didimlog_tests/personal/test_book_state.pytests/didimlog_tests/personal/test_index.pytests/didimlog_tests/project/test_artifacts.pytests/didimlog_tests/project/test_capture.pytests/didimlog_tests/project/test_index.pytests/didimlog_tests/project/test_record.pytests/didimlog_tests/project/test_scaffold.pytests/didimlog_tests/project/test_tree.pytests/didimlog_tests/test_cli_commands.pytests/didimlog_tests/test_file_io.pytests/didimlog_tests/test_locking.pytests/didimlog_tests/test_package_lifecycle.pytests/didimlog_tests/test_retrieval_ab.pytools/didim-retrieval-ab
🚧 Files skipped from review as they are similar to previous changes (15)
- src/didimlog/errors.py
- artifacts/didimlog-v0.0.1-retrieval-ab.json
- tests/didimlog_tests/project/test_artifacts.py
- tests/didimlog_tests/project/test_index.py
- tests/didimlog_tests/claude/test_config.py
- src/didimlog/project/scaffold.py
- src/didimlog/personal/book_state.py
- src/didimlog/project/artifacts.py
- src/didimlog/project/index.py
- src/didimlog/cli.py
- src/didimlog/personal/lesson.py
- src/didimlog/claude/config.py
- tests/didimlog_tests/test_package_lifecycle.py
- src/didimlog/project/tree.py
- tests/didimlog_tests/project/test_tree.py
Drop Improver migration and status handling, preserve unowned Claude bytes, and require canonical current lesson metadata. Constraint: Manage only Didimlog-owned wiring and current lesson schema Rejected: Auto-migrate previous installations | clean cutover requested Confidence: high Scope-risk: moderate
Not-tested: formatter, lint, build, and tests per task constraint
Not-tested: formatter, lint, build, and tests per task constraint
Not-tested: formatter, lint, build, and tests per task constraint
Not-tested: formatter, lint, build, and tests per task constraint
Not-tested: formatter, lint, build, and tests per task constraint
Reject oversized project excludes before writing, make replacement rollback durable, and report Git discovery failures consistently in setup diagnostics. Constraint: Preserve existing fail-soft index and hook behavior Rejected: Broaden Git discovery semantics across all callers | outside PR scope Confidence: high Scope-risk: moderate
…usion feat: choose project knowledge storage during setup
Preserve top-level Claude configuration rollback when managed resource restoration fails. Require an exact, strict checksum manifest for both release artifacts and align installed project guidance with the real didim add flow.
Pin project and Git object database state during publication and verification so path replacement, concurrent edits, and rollback races fail closed without deleting valid records or user changes. Document the resulting safety behavior. Constraint: Preserve create-only records and user-owned bytes under concurrent mutation Rejected: Path-based rechecks alone | vulnerable to namespace replacement and ambient Git state Confidence: high Scope-risk: broad
Keep record, artifact, Git object database, and index operations on validated descriptor identities so namespace swaps fail closed without leaking descriptors or corrupting rollback state. Constraint: Preserve stable error tokens and capture rollback behavior Confidence: high Scope-risk: moderate
Fix final v0.0.1 release blockers
|
@CodeRabbit review |
✅ Action performedReview finished.
|
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)
tests/didimlog_tests/claude/test_config.py (1)
139-139: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winQuote the expected command in the assertions.
plan_settingsbuilds the command withshlex.quote(launcher_text). These three assertions hard-code the unquoted formf"{launcher} hook session-start". The assertions pass only while the temporary directory path contains no shell metacharacter. IfTMPDIRcontains a space or a quote,shlex.quoteadds quoting and these tests fail for an environment reason, not a code reason.Use
shlex.quote(str(launcher))in the expected value.🛡️ Proposed fix for line 139
- "command": f"{launcher} hook session-start", + "command": ( + shlex.quote(str(launcher)) + + " hook session-start" + ),Also applies to: 213-213, 268-268
🤖 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 `@tests/didimlog_tests/claude/test_config.py` at line 139, Update the expected command assertions near the visible “command” entry and the corresponding assertions at the other two referenced locations to build the launcher portion with shlex.quote(str(launcher)) before appending “hook session-start”, matching plan_settings’ quoting behavior.
🧹 Nitpick comments (21)
tests/didimlog_tests/test_release.py (3)
224-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSkip the build test when
uvis absent.The test calls
uv buildwithcheck=True. On a machine withoutuv,subprocess.runraisesFileNotFoundErrorand the test errors.tests/didimlog_tests/claude/test_status.pyalready uses theshutil.whichskip pattern forgit. Apply the same pattern here. Add a timeout as well, because a build can stall.♻️ Proposed change
def test_wheel_and_sdist_follow_the_public_allowlist(self): + if shutil.which("uv") is None: + self.skipTest("uv is required") with tempfile.TemporaryDirectory() as temporary_directory: output = Path(temporary_directory) subprocess.run( ["uv", "build", "--out-dir", str(output)], cwd=REPO, check=True, capture_output=True, text=True, + timeout=300, )Add the import if it is not present:
import shutil🤖 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 `@tests/didimlog_tests/test_release.py` around lines 224 - 233, Update test_wheel_and_sdist_follow_the_public_allowlist to skip when shutil.which("uv") returns no executable, following the existing pattern in test_status.py; import shutil if needed. Add a timeout to the subprocess.run invocation while preserving its checked uv build behavior.
99-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive a clear failure when the workflow step is renamed.
next(...)without a default raisesStopIteration. The test then reports an opaque error instead of the real cause. Supply a default and assert.♻️ Proposed change
- verification_script = next( - step["run"] - for step in workflow["jobs"]["publish"]["steps"] - if step.get("name") == "Verify tag, filenames, and checksums" - ) + verification_script = next( + ( + step["run"] + for step in workflow["jobs"]["publish"]["steps"] + if step.get("name") == "Verify tag, filenames, and checksums" + ), + None, + ) + self.assertIsNotNone( + verification_script, + "publish job has no 'Verify tag, filenames, and checksums' step", + )🤖 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 `@tests/didimlog_tests/test_release.py` around lines 99 - 103, Update the verification_script lookup in the release workflow test to provide a missing-step default, then explicitly assert that the “Verify tag, filenames, and checksums” step was found before using its run command, producing a clear failure if the workflow step is renamed.
143-149: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a 60-second timeout to the verification subprocess.
The workflow script already enables
pipefail, so no additional Bash flags are needed.🤖 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 `@tests/didimlog_tests/test_release.py` around lines 143 - 149, Add a 60-second timeout to the subprocess invocation in the verification flow using subprocess.run’s timeout parameter, while preserving the existing command, environment, output capture, and Bash flags.tests/didimlog_tests/claude/test_status.py (1)
179-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the version instead of hardcoding
0.0.1.The expected text embeds the release version. The next version bump breaks this test for a reason unrelated to status behavior. Read the installed version with
importlib.metadata.version("didimlog"), or import the same constant thatstatus_textuses.♻️ Proposed change
+from importlib.metadata import version as package_version + def test_healthy_status_summarizes_current_surfaces(self): text = self._status() self.assertEqual( text, - "Didimlog 0.0.1\n" + f"Didimlog {package_version('didimlog')}\n" "개인 지식: 최신\n" "현재 프로젝트: demo-project\n" "프로젝트 근거: 최신\n" "Claude 연결: 정상\n", )🤖 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 `@tests/didimlog_tests/claude/test_status.py` around lines 179 - 189, Update test_healthy_status_summarizes_current_surfaces to derive the expected Didimlog version from importlib.metadata.version("didimlog") or the same version constant used by status_text, and interpolate that value into the expected text instead of hardcoding "0.0.1".tests/didimlog_tests/test_cli_commands.py (1)
152-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssertions run outside
subTestin four parameterized tests. In each test the assertion block is dedented out of thewith self.subTest(...), mock.patch(...)statement. The assertions still read valid recorded mock state, so the tests pass today. However, a failure is not attributed to the failing case, and the loop aborts instead of running the remaining cases. Indent each assertion block into itswithstatement.
tests/didimlog_tests/test_cli_commands.py#L152-L163: indent the assertions into thewith self.subTest(selected=selected), mock.patch(...)block on line 137.tests/didimlog_tests/test_cli_commands.py#L202-L207: indent the assertions into thewith self.subTest(mode=mode), mock.patch(...)block on line 188.tests/didimlog_tests/test_cli_commands.py#L227-L233: indent the assertions into thewith self.subTest(name=name), mock.patch(...)block on line 216.tests/didimlog_tests/test_cli_commands.py#L417-L418: indent the assertions into thewith self.subTest(token=token), mock.patch(...)block on line 399.🤖 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 `@tests/didimlog_tests/test_cli_commands.py` around lines 152 - 163, Assertions in four parameterized test cases are outside their subTest and patch contexts, preventing failures from being attributed to individual cases. In tests/didimlog_tests/test_cli_commands.py:152-163, 202-207, 227-233, and 417-418, indent each assertion block into its corresponding with self.subTest(...), mock.patch(...) block so every iteration runs and reports independently.tests/didimlog_tests/test_indexing.py (1)
27-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine
_legacy_readmeonce in a shared test helper.The identical helper is duplicated in
tests/didimlog_tests/test_indexing.py,tests/didimlog_tests/project/test_scaffold.py, andtests/didimlog_tests/project/test_capture.py. Move the helper, expected length, and SHA-256 digest into one shared module, then import it in all three files.🤖 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 `@tests/didimlog_tests/test_indexing.py` around lines 27 - 66, Move the duplicated _legacy_readme helper, including its expected length and SHA-256 assertion, into a shared test helper module. Remove the local definitions from test_indexing.py, project/test_scaffold.py, and project/test_capture.py, and import the shared _legacy_readme in each file while preserving its existing behavior.tests/didimlog_tests/project/test_git_exclude.py (1)
28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
_MAXIMUM_EXCLUDE_BYTESfromdidimlog.project.git_exclude.The boundary tests duplicate the production limit. Use the production constant for the exact-limit and one-byte-over cases.
🤖 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 `@tests/didimlog_tests/project/test_git_exclude.py` at line 28, Update the boundary tests in the relevant test module to import and use _MAXIMUM_EXCLUDE_BYTES from didimlog.project.git_exclude instead of defining the duplicate MAXIMUM_EXCLUDE_BYTES value. Ensure the exact-limit and one-byte-over cases derive their inputs from the production constant.src/didimlog/project/git_exclude.py (1)
222-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op
try/except.Lines 222-225 catch
DidimErrorand immediately re-raise it. The block changes nothing. It suggests thatDidimErroris handled here, which it is not.♻️ Proposed cleanup
- try: - root = _strict_path(result.stdout) - except DidimError: - raise + root = _strict_path(result.stdout) candidate = _root_candidate(source, root)🤖 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 `@src/didimlog/project/git_exclude.py` around lines 222 - 229, Remove the redundant try/except around _strict_path(result.stdout) in the surrounding candidate-resolution flow, allowing DidimError to propagate directly while preserving the existing _root_candidate and _git_unavailable handling.tests/didimlog_tests/claude/test_connect.py (1)
460-494: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBind the loop variables in the two closures.
Ruff reports B023 at lines 466, 471, 482, 483, 484, 492, and 493. Both closures read
removal_to_restore,original_write,raced_path,replacement_path,original_replace, andconditional_publish_resultsfrom the enclosing loop scope. The current test passes because each closure is created and used inside one iteration. The late binding becomes a real defect if a later change moves themock.patchscope outside the loop.Bind the values as default arguments, or extract the per-case body into a helper method.
♻️ Proposed fix using default arguments
def fail_deleted_resource_restoration( path: Path, expected: bytes | None, intended: bytes | None, + *, + removal_to_restore=removal_to_restore, + original_write=original_write, ) -> None: nonlocal restoration_attempts @@ def replace_target_before_conditional_publish( parent_descriptor: int, name: str, expected: bytes, replacement: bytes, mode: int, *, expected_info=None, + raced_path=raced_path, + replacement_path=replacement_path, + original_replace=original_replace, + conditional_publish_results=conditional_publish_results, ) -> bool:🤖 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 `@tests/didimlog_tests/claude/test_connect.py` around lines 460 - 494, Update the closures fail_deleted_resource_restoration and replace_target_before_conditional_publish to bind all loop-scoped values they read—removal_to_restore, original_write, raced_path, replacement_path, original_replace, and conditional_publish_results—as default arguments or through a per-case helper, while preserving their current behavior.Source: Linters/SAST tools
src/didimlog/claude/setup.py (1)
292-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated project-index guard.
Lines 292-300 and lines 312-320 contain identical logic. The second check is required because
inspectcan observe a different repository, so both calls must stay in place. Keep both calls, but move the shared body into one helper. This prevents the two guards from diverging in a later change.♻️ Proposed refactor
+def _project_index_is_current(project_root: Path | None) -> bool: + if project_root is None: + return True + try: + return _prepared_project(project_root) and _project_check( + project_root + ) == PROJECT_INDEX_CURRENT + except (DidimError, OSError, ValueError): + return False + + def _postcheck(plan: SetupPlan) -> tuple[str, ...]: if _personal_check(plan._personal.root) != PERSONAL_INDEX_CURRENT: raise DidimError("SETUP_POSTCHECK_FAILED", exit_code=EXIT_POLICY) - if plan._project_root is not None: - try: - project_current = _prepared_project( - plan._project_root - ) and _project_check(plan._project_root) == PROJECT_INDEX_CURRENT - except (DidimError, OSError, ValueError): - project_current = False - if not project_current: - raise DidimError("SETUP_POSTCHECK_FAILED", exit_code=EXIT_POLICY) + if not _project_index_is_current(plan._project_root): + raise DidimError("SETUP_POSTCHECK_FAILED", exit_code=EXIT_POLICY) if plan._claude is not None: problems = inspect( home=plan._personal.home, cwd=plan._project_root, config=plan._claude.config_dir, ) if any( not problem.token.startswith("PROJECT_INDEX_") for problem in problems ): raise DidimError("SETUP_POSTCHECK_FAILED", exit_code=EXIT_POLICY) - if plan._project_root is not None: - try: - project_current = _prepared_project( - plan._project_root - ) and _project_check(plan._project_root) == PROJECT_INDEX_CURRENT - except (DidimError, OSError, ValueError): - project_current = False - if not project_current: - raise DidimError("SETUP_POSTCHECK_FAILED", exit_code=EXIT_POLICY) - + if not _project_index_is_current(plan._project_root): + raise DidimError("SETUP_POSTCHECK_FAILED", exit_code=EXIT_POLICY)🤖 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 `@src/didimlog/claude/setup.py` around lines 292 - 320, Extract the duplicated project-index validation body into a shared helper near the setup postcheck logic, including the _prepared_project/_project_check calls, exception handling, and SETUP_POSTCHECK_FAILED raise. Replace both existing inline guards with calls to that helper, preserving both validation calls after their respective repository checks.tests/didimlog_tests/claude/test_setup_plan.py (1)
14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the Git test fixtures with
tests/didimlog_tests/claude/test_setup_apply.py.The constants
GIT,START,RULE,END, andLOCAL_BLOCK, plusgit_environment,_git,_exclude_path,_plan, and_snapshot, are duplicated intests/didimlog_tests/claude/test_setup_apply.py. The two copies already differ:_snapshotrecordsst_modehere but not in the other module, and the Git repository is created in_git_projecthere but insetUpthere.Move the shared constants and helpers into one test support module, and import them in both test files. This stops the two fixtures from drifting further.
Also applies to: 36-41, 43-66, 87-114
🤖 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 `@tests/didimlog_tests/claude/test_setup_plan.py` around lines 14 - 18, Move the duplicated fixtures GIT, START, RULE, END, LOCAL_BLOCK and helpers git_environment, _git, _exclude_path, _plan, and _snapshot into a shared test support module; then import and reuse them from both test_setup_plan.py and test_setup_apply.py. Preserve the existing fixture behavior, including consistent st_mode snapshot handling and Git repository setup, while removing the duplicate definitions from each test module.src/didimlog/project/tree.py (1)
391-406: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe pinned-snapshot contract is duck-typed across two separate
_PinnedWorkspacePathclasses.Lines 402-405 read
workspace_descriptorandknowledge_descriptorwithgetattr. Two independent classes satisfy this contract: the frozen dataclass insrc/didimlog/project/capture.pyat lines 81-88 and the__slots__class insrc/didimlog/project/index.pyat lines 22-40. Neither declares the relationship, andindex._PinnedWorkspacePathallowsknowledge_descriptor=Nonewhile the capture version requires anint.The current call sites work. A future change to one class will not be caught by the other. Consider one shared definition, or a
Protocolthat both satisfy, so the contract is explicit.🤖 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 `@src/didimlog/project/tree.py` around lines 391 - 406, Make the pinned workspace path contract explicit instead of relying on duplicate duck-typed classes. Define one shared _PinnedWorkspacePath or a Protocol exposing workspace_descriptor and knowledge_descriptor, then update capture._PinnedWorkspacePath and index._PinnedWorkspacePath to use it while preserving the existing optional knowledge_descriptor behavior.src/didimlog/project/scaffold.py (1)
327-363: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe rollback path acquires a blocking directory lock.
Line 352 calls
acquire_directory_lock(parent_descriptor). That helper defaults toblocking=True._rollback_scaffold_updateruns only while an exception propagates. If another process holds the exclusive lock on the same directory at that moment, the rollback blocks until the other process releases it, and the original error surfaces only after the wait.Consider a non-blocking attempt with a bounded retry. If the lock cannot be taken, skip the restore, which is already the fail-safe outcome for the other cleanup branches.
🔧 Proposed change
- lock_descriptor = acquire_directory_lock(parent_descriptor) + try: + lock_descriptor = acquire_directory_lock( + parent_descriptor, + blocking=False, + ) + except BlockingIOError: + return try: replace_regular_file_at_if_unchanged(🤖 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 `@src/didimlog/project/scaffold.py` around lines 327 - 363, Update _rollback_scaffold_update to acquire the directory lock non-blockingly with a bounded retry attempt; if the lock cannot be obtained within that bound, skip the restore and allow the original exception to propagate without waiting indefinitely. Preserve the existing replace_regular_file_at_if_unchanged cleanup when the lock is acquired, including descriptor closure.src/didimlog/project/index.py (2)
599-625: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueA failed post-rollback
fsyncleaves a stray.baklink in the index directory.At line 616 the code calls
os.fsync(directory_descriptor). If that call raisesOSError, line 618 setsbackup_name = None. The block at lines 621-625 then skips the unlink, so the.didim-index-<hex>.bakhard link stays inknowledge/index/.The retained link is intentional insurance when durability is unconfirmed. Nothing later removes it, so the directory accumulates one file per failed rollback. Consider documenting this in a comment, or removing stale
.baklinks on the next successful publication.🤖 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 `@src/didimlog/project/index.py` around lines 599 - 625, Update the cleanup logic in the publication rollback finally block so a failed post-rollback os.fsync does not permanently skip removal of the temporary backup link. Ensure stale .didim-index-.bak links are removed on the next successful publication, while preserving the backup when rollback or durability remains unconfirmed.
22-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRuff reports
RUF023on__slots__.Ruff flags the
__slots__tuple as unsorted. If the project enables theRUFrule set in CI, this fails the lint job. Sorting the entries resolves it without behavior change.🔧 Proposed change
class _PinnedWorkspacePath: __slots__ = ( - "path", - "workspace_descriptor", "knowledge_descriptor", + "path", + "workspace_descriptor", )#!/bin/bash # Description: Check whether Ruff RUF rules are enforced for this repository. set -euo pipefail fd -t f -H 'pyproject.toml|ruff.toml|.ruff.toml' -d 2 | xargs -r rg -n -A 20 '\[tool\.ruff' fd -t f 'ci.yml' .github | xargs -r rg -n -C 3 'ruff'🤖 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 `@src/didimlog/project/index.py` around lines 22 - 40, Sort the entries in the __slots__ tuple of _PinnedWorkspacePath alphabetically to satisfy Ruff RUF023, preserving all existing slot names and behavior.Source: Linters/SAST tools
tests/didimlog_tests/project/test_capture.py (1)
1253-1259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe 0.3-second wait makes this test timing sensitive.
Line 1257 uses
capture_reached_publish.wait(0.3). On a loaded CI runner the capture thread may not reach_write_create_onlywithin 300 milliseconds. The old-index writer then proceeds without pausing, and the test no longer exercises the intended interleaving. The assertions at lines 1305-1309 can still pass, so the test would silently stop testing the race instead of failing.Consider recording whether the pause actually happened and asserting on it, so a missed interleaving reports a failure.
🤖 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 `@tests/didimlog_tests/project/test_capture.py` around lines 1253 - 1259, The pause_old_snapshot helper currently ignores whether capture_reached_publish.wait(0.3) succeeded, allowing the race test to continue without exercising the intended interleaving. Record the wait result in shared test state and add an assertion in the test that the pause was reached, while preserving the existing capture_finished synchronization.tests/didimlog_tests/project/test_index.py (1)
749-798: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIsolate the Git environment for this test.
Lines 757-758 call
git initwith noenvargument, so the commands read the developer's global and system Git configuration. Lines 764-772 buildgit_environmentfromos.environand add only the four identity variables, so theadd,commit, andrev-parsecalls at lines 773-798 also read that configuration.A global
core.hooksPath,init.templateDir, orcore.symlinkssetting changes what these commands produce. The test then asserts on repository state it did not fully control. SetHOMEto a temporary directory and setGIT_CONFIG_NOSYSTEM=1, and pass the environment to every Git call, matching_seed_repositoryintests/didimlog_tests/project/test_artifacts.py.The Ruff
S603andS607hints on these lines are expected for test fixtures that invokegitby name.🔧 Proposed change
workspace.mkdir() replacement.mkdir() - subprocess.run(["git", "init", "-q", str(workspace)], check=True) - subprocess.run(["git", "init", "-q", str(replacement)], check=True) + home = root / "home" + home.mkdir() + git_environment = os.environ.copy() + git_environment.update( + { + "HOME": str(home), + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_AUTHOR_NAME": "Didimlog Tests", + "GIT_AUTHOR_EMAIL": "didimlog-tests@example.invalid", + "GIT_COMMITTER_NAME": "Didimlog Tests", + "GIT_COMMITTER_EMAIL": "didimlog-tests@example.invalid", + } + ) + subprocess.run( + ["git", "init", "-q", str(workspace)], + check=True, + env=git_environment, + ) + subprocess.run( + ["git", "init", "-q", str(replacement)], + check=True, + env=git_environment, + ) artifact_path = "knowledge/raw/data/replacement-only.bin" artifact = replacement / artifact_path artifact.parent.mkdir(parents=True) artifact.write_bytes(b"replacement-only artifact\n") - git_environment = os.environ.copy() - git_environment.update( - { - "GIT_AUTHOR_NAME": "Didimlog Tests", - "GIT_AUTHOR_EMAIL": "didimlog-tests@example.invalid", - "GIT_COMMITTER_NAME": "Didimlog Tests", - "GIT_COMMITTER_EMAIL": "didimlog-tests@example.invalid", - } - )Also pass
env=git_environmentto therev-parsecall at lines 793-798.🤖 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 `@tests/didimlog_tests/project/test_index.py` around lines 749 - 798, Isolate every Git subprocess in test_public_index_surfaces_reject_external_git_alternates from developer and system configuration. Extend git_environment with a temporary HOME and GIT_CONFIG_NOSYSTEM=1, then pass env=git_environment to both git init calls and the existing add, commit, and rev-parse calls, matching _seed_repository’s setup while preserving the test fixture behavior.Source: Linters/SAST tools
tests/didimlog_tests/project/test_scaffold.py (1)
48-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe hash-pinned
_legacy_readmefixture is defined twice. Both modules contain a byte-identical helper that reconstructs the legacy README, then asserts an exact length of16_336and an exact SHA-256 digest. That digest must match_LEGACY_README_SHA256insrc/didimlog/project/scaffold.pyat lines 42-45. Any README text change requires editing every copy, and a missed copy produces a confusing assertion failure inside the fixture rather than in the test under review.
tests/didimlog_tests/project/test_scaffold.py#L48-L88: move this definition into a shared test helper module and import it here.tests/didimlog_tests/project/test_capture.py#L38-L78: delete this copy and import the shared helper.🤖 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 `@tests/didimlog_tests/project/test_scaffold.py` around lines 48 - 88, Move the duplicate _legacy_readme helper from tests/didimlog_tests/project/test_scaffold.py lines 48-88 into a shared test helper module, preserving its replacement logic, length assertion, and SHA-256 validation; import it in test_scaffold.py. Delete the duplicate definition from tests/didimlog_tests/project/test_capture.py lines 38-78 and import the shared helper there.src/didimlog/project/capture.py (2)
608-609: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op
except BaseException: raise.The clause catches every exception and immediately re-raises it. The
finallyblock at lines 610-618 already performs all cleanup. Deleting the clause does not change behavior.♻️ Proposed cleanup
return publication - except BaseException: - raise finally: if temporary_descriptor is not None: os.close(temporary_descriptor)🤖 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 `@src/didimlog/project/capture.py` around lines 608 - 609, Remove the no-op `except BaseException: raise` clause from the surrounding capture flow, leaving the existing `finally` cleanup block and exception propagation behavior unchanged.
639-645: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_require_record_rollbackalways raises, so the surroundingraisestatements never run.
_require_record_rollbackunconditionally raises_workspace_replaced_error(). The callers wrap it intry: ... finally: _refresh_index(...), so the new error propagates immediately. Theraiseat line 798 and line 830 cannot execute. The parameterstarget,publication, andintendedare also unused.The resulting token is
PROJECT_SCAFFOLD_MISSINGin both paths, so behavior is unchanged. The structure hides that fact from a reader. Consider replacing the call with a direct_refresh_indexcall followed byraise, or keep the helper and drop the unreachableraise.Also applies to: 785-798, 817-830
🤖 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 `@src/didimlog/project/capture.py` around lines 639 - 645, Simplify the rollback failure paths around callers of _require_record_rollback: remove the unreachable raise statements after this helper, or replace the helper invocation with a direct _refresh_index call followed by the intended raise. Ensure both affected paths preserve the PROJECT_SCAFFOLD_MISSING error behavior and remove the unused helper parameters if the helper remains.src/didimlog/project/artifacts.py (1)
979-1095: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
finallyblock can replace the original failure token.Lines 1089-1095 call
_require_workspace_identityinsidefinally. If the workspace identity changed while anARTIFACT_GIT_PATHorGIT_UNVERIFIABLEerror was already propagating, the newARTIFACT_PATH_ESCAPEPolicyErrorreplaces it. The exit code then changes from 7 to 3 for the Git-unavailable case.This behavior is fail-closed and safe. It is only a diagnosis concern: the caller loses the first cause. Consider chaining the original error, or documenting that the final identity check takes precedence over the in-flight token.
🤖 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 `@src/didimlog/project/artifacts.py` around lines 979 - 1095, Adjust the finally-path identity check in verify_artifact_git so an exception already propagating from artifact or Git validation is not silently replaced by a later _require_workspace_identity failure. Preserve the final identity validation, but chain or otherwise retain the original exception while documenting or enforcing the intended precedence.
🤖 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 `@src/didimlog/conditional_file.py`:
- Around line 214-220: Update the os.link call in the conditional file
publication logic to support macOS versions before 10.10: either declare macOS
10.10 as the minimum supported version or provide a fallback when
src_dir_fd/dst_dir_fd raise NotImplementedError. Preserve atomic target
publication and the existing follow_symlinks=False behavior.
In `@src/didimlog/project/git_exclude.py`:
- Around line 427-479: Document Git 1.8.5 or newer as a prerequisite for the
_planned_knowledge_is_ignored flow, because it invokes check-ignore with
--no-index. Update the project’s appropriate requirements or setup
documentation, explicitly noting that older Git versions are unsupported and can
cause didim setup to report PROJECT_EXCLUDE_GIT_UNAVAILABLE.
---
Outside diff comments:
In `@tests/didimlog_tests/claude/test_config.py`:
- Line 139: Update the expected command assertions near the visible “command”
entry and the corresponding assertions at the other two referenced locations to
build the launcher portion with shlex.quote(str(launcher)) before appending
“hook session-start”, matching plan_settings’ quoting behavior.
---
Nitpick comments:
In `@src/didimlog/claude/setup.py`:
- Around line 292-320: Extract the duplicated project-index validation body into
a shared helper near the setup postcheck logic, including the
_prepared_project/_project_check calls, exception handling, and
SETUP_POSTCHECK_FAILED raise. Replace both existing inline guards with calls to
that helper, preserving both validation calls after their respective repository
checks.
In `@src/didimlog/project/artifacts.py`:
- Around line 979-1095: Adjust the finally-path identity check in
verify_artifact_git so an exception already propagating from artifact or Git
validation is not silently replaced by a later _require_workspace_identity
failure. Preserve the final identity validation, but chain or otherwise retain
the original exception while documenting or enforcing the intended precedence.
In `@src/didimlog/project/capture.py`:
- Around line 608-609: Remove the no-op `except BaseException: raise` clause
from the surrounding capture flow, leaving the existing `finally` cleanup block
and exception propagation behavior unchanged.
- Around line 639-645: Simplify the rollback failure paths around callers of
_require_record_rollback: remove the unreachable raise statements after this
helper, or replace the helper invocation with a direct _refresh_index call
followed by the intended raise. Ensure both affected paths preserve the
PROJECT_SCAFFOLD_MISSING error behavior and remove the unused helper parameters
if the helper remains.
In `@src/didimlog/project/git_exclude.py`:
- Around line 222-229: Remove the redundant try/except around
_strict_path(result.stdout) in the surrounding candidate-resolution flow,
allowing DidimError to propagate directly while preserving the existing
_root_candidate and _git_unavailable handling.
In `@src/didimlog/project/index.py`:
- Around line 599-625: Update the cleanup logic in the publication rollback
finally block so a failed post-rollback os.fsync does not permanently skip
removal of the temporary backup link. Ensure stale .didim-index-.bak links are
removed on the next successful publication, while preserving the backup when
rollback or durability remains unconfirmed.
- Around line 22-40: Sort the entries in the __slots__ tuple of
_PinnedWorkspacePath alphabetically to satisfy Ruff RUF023, preserving all
existing slot names and behavior.
In `@src/didimlog/project/scaffold.py`:
- Around line 327-363: Update _rollback_scaffold_update to acquire the directory
lock non-blockingly with a bounded retry attempt; if the lock cannot be obtained
within that bound, skip the restore and allow the original exception to
propagate without waiting indefinitely. Preserve the existing
replace_regular_file_at_if_unchanged cleanup when the lock is acquired,
including descriptor closure.
In `@src/didimlog/project/tree.py`:
- Around line 391-406: Make the pinned workspace path contract explicit instead
of relying on duplicate duck-typed classes. Define one shared
_PinnedWorkspacePath or a Protocol exposing workspace_descriptor and
knowledge_descriptor, then update capture._PinnedWorkspacePath and
index._PinnedWorkspacePath to use it while preserving the existing optional
knowledge_descriptor behavior.
In `@tests/didimlog_tests/claude/test_connect.py`:
- Around line 460-494: Update the closures fail_deleted_resource_restoration and
replace_target_before_conditional_publish to bind all loop-scoped values they
read—removal_to_restore, original_write, raced_path, replacement_path,
original_replace, and conditional_publish_results—as default arguments or
through a per-case helper, while preserving their current behavior.
In `@tests/didimlog_tests/claude/test_setup_plan.py`:
- Around line 14-18: Move the duplicated fixtures GIT, START, RULE, END,
LOCAL_BLOCK and helpers git_environment, _git, _exclude_path, _plan, and
_snapshot into a shared test support module; then import and reuse them from
both test_setup_plan.py and test_setup_apply.py. Preserve the existing fixture
behavior, including consistent st_mode snapshot handling and Git repository
setup, while removing the duplicate definitions from each test module.
In `@tests/didimlog_tests/claude/test_status.py`:
- Around line 179-189: Update test_healthy_status_summarizes_current_surfaces to
derive the expected Didimlog version from importlib.metadata.version("didimlog")
or the same version constant used by status_text, and interpolate that value
into the expected text instead of hardcoding "0.0.1".
In `@tests/didimlog_tests/project/test_capture.py`:
- Around line 1253-1259: The pause_old_snapshot helper currently ignores whether
capture_reached_publish.wait(0.3) succeeded, allowing the race test to continue
without exercising the intended interleaving. Record the wait result in shared
test state and add an assertion in the test that the pause was reached, while
preserving the existing capture_finished synchronization.
In `@tests/didimlog_tests/project/test_git_exclude.py`:
- Line 28: Update the boundary tests in the relevant test module to import and
use _MAXIMUM_EXCLUDE_BYTES from didimlog.project.git_exclude instead of defining
the duplicate MAXIMUM_EXCLUDE_BYTES value. Ensure the exact-limit and
one-byte-over cases derive their inputs from the production constant.
In `@tests/didimlog_tests/project/test_index.py`:
- Around line 749-798: Isolate every Git subprocess in
test_public_index_surfaces_reject_external_git_alternates from developer and
system configuration. Extend git_environment with a temporary HOME and
GIT_CONFIG_NOSYSTEM=1, then pass env=git_environment to both git init calls and
the existing add, commit, and rev-parse calls, matching _seed_repository’s setup
while preserving the test fixture behavior.
In `@tests/didimlog_tests/project/test_scaffold.py`:
- Around line 48-88: Move the duplicate _legacy_readme helper from
tests/didimlog_tests/project/test_scaffold.py lines 48-88 into a shared test
helper module, preserving its replacement logic, length assertion, and SHA-256
validation; import it in test_scaffold.py. Delete the duplicate definition from
tests/didimlog_tests/project/test_capture.py lines 38-78 and import the shared
helper there.
In `@tests/didimlog_tests/test_cli_commands.py`:
- Around line 152-163: Assertions in four parameterized test cases are outside
their subTest and patch contexts, preventing failures from being attributed to
individual cases. In tests/didimlog_tests/test_cli_commands.py:152-163, 202-207,
227-233, and 417-418, indent each assertion block into its corresponding with
self.subTest(...), mock.patch(...) block so every iteration runs and reports
independently.
In `@tests/didimlog_tests/test_indexing.py`:
- Around line 27-66: Move the duplicated _legacy_readme helper, including its
expected length and SHA-256 assertion, into a shared test helper module. Remove
the local definitions from test_indexing.py, project/test_scaffold.py, and
project/test_capture.py, and import the shared _legacy_readme in each file while
preserving its existing behavior.
In `@tests/didimlog_tests/test_release.py`:
- Around line 224-233: Update test_wheel_and_sdist_follow_the_public_allowlist
to skip when shutil.which("uv") returns no executable, following the existing
pattern in test_status.py; import shutil if needed. Add a timeout to the
subprocess.run invocation while preserving its checked uv build behavior.
- Around line 99-103: Update the verification_script lookup in the release
workflow test to provide a missing-step default, then explicitly assert that the
“Verify tag, filenames, and checksums” step was found before using its run
command, producing a clear failure if the workflow step is renamed.
- Around line 143-149: Add a 60-second timeout to the subprocess invocation in
the verification flow using subprocess.run’s timeout parameter, while preserving
the existing command, environment, output capture, and Bash flags.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3da84c21-c228-4ec9-9daa-e8091b7e54bb
📒 Files selected for processing (49)
.github/workflows/release.ymlCHANGELOG.mdREADME.mdsrc/didimlog/claude/config.pysrc/didimlog/claude/connect.pysrc/didimlog/claude/probe.pysrc/didimlog/claude/setup.pysrc/didimlog/claude/status.pysrc/didimlog/claude/transaction.pysrc/didimlog/cli.pysrc/didimlog/conditional_file.pysrc/didimlog/file_io.pysrc/didimlog/indexing.pysrc/didimlog/personal/index.pysrc/didimlog/personal/lesson.pysrc/didimlog/project/artifacts.pysrc/didimlog/project/capture.pysrc/didimlog/project/git_exclude.pysrc/didimlog/project/index.pysrc/didimlog/project/scaffold.pysrc/didimlog/project/tree.pysrc/didimlog/resources/project/README.mdsrc/didimlog/resources/project/record.schema.jsontests/didimlog_tests/claude/test_config.pytests/didimlog_tests/claude/test_connect.pytests/didimlog_tests/claude/test_setup_apply.pytests/didimlog_tests/claude/test_setup_plan.pytests/didimlog_tests/claude/test_status.pytests/didimlog_tests/personal/fixtures/demo-api-cache.mdtests/didimlog_tests/personal/fixtures/expired-boundary.mdtests/didimlog_tests/personal/fixtures/jpa-n-plus-one.mdtests/didimlog_tests/personal/fixtures/kafka-idempotence.mdtests/didimlog_tests/personal/fixtures/korean-lesson.mdtests/didimlog_tests/personal/fixtures/malformed-no-close.mdtests/didimlog_tests/personal/fixtures/multiline-attempt.mdtests/didimlog_tests/personal/test_index.pytests/didimlog_tests/personal/test_lesson.pytests/didimlog_tests/project/test_artifacts.pytests/didimlog_tests/project/test_capture.pytests/didimlog_tests/project/test_git_exclude.pytests/didimlog_tests/project/test_index.pytests/didimlog_tests/project/test_scaffold.pytests/didimlog_tests/project/test_tree.pytests/didimlog_tests/test_cli_commands.pytests/didimlog_tests/test_conditional_file.pytests/didimlog_tests/test_file_io.pytests/didimlog_tests/test_indexing.pytests/didimlog_tests/test_locking.pytests/didimlog_tests/test_release.py
💤 Files with no reviewable changes (8)
- tests/didimlog_tests/personal/fixtures/korean-lesson.md
- tests/didimlog_tests/personal/fixtures/expired-boundary.md
- tests/didimlog_tests/personal/fixtures/multiline-attempt.md
- tests/didimlog_tests/personal/fixtures/malformed-no-close.md
- tests/didimlog_tests/personal/fixtures/demo-api-cache.md
- tests/didimlog_tests/personal/fixtures/kafka-idempotence.md
- tests/didimlog_tests/personal/fixtures/jpa-n-plus-one.md
- tests/didimlog_tests/test_locking.py
🚧 Files skipped from review as they are similar to previous changes (10)
- src/didimlog/resources/project/record.schema.json
- tests/didimlog_tests/personal/test_lesson.py
- tests/didimlog_tests/personal/test_index.py
- .github/workflows/release.yml
- src/didimlog/indexing.py
- src/didimlog/personal/index.py
- src/didimlog/claude/connect.py
- src/didimlog/file_io.py
- tests/didimlog_tests/project/test_tree.py
- src/didimlog/resources/project/README.md
| os.link( | ||
| temporary_name, | ||
| target.name, | ||
| src_dir_fd=parent_descriptor, | ||
| dst_dir_fd=parent_descriptor, | ||
| follow_symlinks=False, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Python os.link follow_symlinks False NotImplementedError platform support
💡 Result:
In Python, the support for the follow_symlinks argument in os.link() varies significantly by platform and Python version due to differences in underlying system calls [1][2][3]. Platform Support and Implementation: - The behavior of os.link(src, dst, *, follow_symlinks=True) depends on the platform's link() system call [4][2][3]. Historically, many implementations simply mapped os.link() directly to the platform's native link() function, leading to inconsistent behaviors where some platforms followed symlinks and others did not [2][3]. - Modern Python versions (targeted at 3.14+) have moved toward using the linkat() system call where available to provide consistent, cross-platform behavior [1][4]. Regarding NotImplementedError: - If a platform does not support the requested follow_symlinks behavior and the Python implementation cannot guarantee it (or if the required system call like linkat() is unavailable at runtime), os.link() may raise a NotImplementedError or an OSError (e.g., PermissionError or generic system error) depending on the specific environment and Python version [1][4]. - You can programmatically check for support on your current platform by using os.supports_follow_symlinks, which is a sequence of function objects that support the follow_symlinks argument [5][6]. If a function is not in this sequence, using the parameter may raise NotImplementedError on that specific platform [5][6]. Key Version Changes: - Efforts have been made (notably in Python 3.14) to improve this by ensuring os.link() uses linkat() whenever possible [1][4]. - On Windows, os.link() does not support follow_symlinks=True and will typically raise an error if this is requested, as the native CreateHardLinkW function behaves as if follow_symlinks=False [4]. - On macOS, requesting follow_symlinks=False will raise an error if the system linkat() function is not available at runtime [1][4]. In summary, reliance on follow_symlinks=False in os.link() is not universally portable across all platforms and versions [2][3]. If you need portable behavior, it is often safer to resolve symlinks manually using os.readlink() or pathlib before calling os.link() [7].
Citations:
- 1: gh-81793: Always call linkat() from os.link(), if available python/cpython#132517
- 2: Wrong
follow_symlinksdefault fallback implementation python/cpython#119885 - 3: os.link(..., follow_symlinks=False) without linkat(3) python/cpython#85527
- 4: os.link(..., follow_symlinks=True) broken on Linux python/cpython#81793
- 5: https://docs.python.org/3/library/os.html
- 6: https://docs.pythonlang.net/3/library/os.html
- 7: Clarify os.link behavior with follow_symlinks=True on POSIX systems python/cpython#138623
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- repository files ---'
git ls-files | sed -n '1,120p'
printf '%s\n' '--- project metadata ---'
for f in pyproject.toml setup.py setup.cfg tox.ini .github/workflows/*.yml .github/workflows/*.yaml; do
if [ -f "$f" ]; then
printf '\n### %s\n' "$f"
sed -n '1,240p' "$f"
fi
done
printf '%s\n' '--- target implementation ---'
sed -n '180,240p' src/didimlog/conditional_file.py
printf '%s\n' '--- support checks and related calls ---'
rg -n -C 3 'supports_follow_symlinks|os\.link|follow_symlinks|python_requires|requires-python|python-version|platform' .Repository: zhsks311/didimlog
Length of output: 50374
🌐 Web query:
CPython os.link follow_symlinks False dir_fd macOS Linux Python 3.11 3.12 3.13 3.14 NotImplementedError
💡 Result:
In CPython, the behavior of os.link regarding the follow_symlinks and directory descriptor (dir_fd) arguments has historically been inconsistent and platform-dependent, as it relied on the underlying operating system's link(2) system call [1][2]. For Python versions 3.11, 3.12, and 3.13, the support for follow_symlinks and the dir_fd variants (src_dir_fd, dst_dir_fd) is largely determined by the availability of the linkat(2) system call at build time and runtime [1][2]. If a platform does not support the requested operation (e.g., if linkat is missing or if the system link call does not support flags), CPython may raise a NotImplementedError or, more commonly, ignore the argument and exhibit platform-specific behavior (e.g., silently following or not following symlinks depending on the kernel) [1][3][2]. However, substantial changes were introduced for Python 3.14 (tracking issue GH-81793/PR-132517) to standardize this behavior by attempting to use linkat whenever it is available [4][1][5]. Under these recent changes: 1. Consistency: CPython now aims to use linkat to consistently honor the follow_symlinks parameter across platforms [4][5]. 2. Linux: The implementation now correctly follows symlinks by default (and when follow_symlinks=True), and honors follow_symlinks=False [4][1][5]. 3. macOS: If follow_symlinks=False is requested but the system's linkat function is not available at runtime, an error will be raised [4][5]. 4. Windows: Specifying follow_symlinks=True now raises an error, as this is not supported [4][5]. If you encounter a NotImplementedError or unexpected behavior in older versions (3.11-3.13), it is often due to the platform-specific limitations of the system link call [1][2]. Developers are advised to check os.supports_follow_symlinks to verify capability at runtime [6][7]. In environments where symlink handling is critical and consistent behavior is required, explicitly verifying the symlink target or using file descriptors via open may be necessary [8].
Citations:
- 1: os.link(..., follow_symlinks=True) broken on Linux python/cpython#81793
- 2: Wrong
follow_symlinksdefault fallback implementation python/cpython#119885 - 3: os.link(..., follow_symlinks=False) without linkat(3) python/cpython#85527
- 4: gh-81793: Always call linkat() from os.link(), if available python/cpython#132517
- 5: python/cpython@5a57248
- 6: https://docs.python.org/3/library/os.html
- 7: https://docs.python.org/3.11/library/os.html
- 8: Clarify os.link behavior with follow_symlinks=True on POSIX systems python/cpython#138623
🌐 Web query:
site:docs.python.org/3.11/library/os.html os.link supports_follow_symlinks follow_symlinks
💡 Result:
In Python, the os.link function supports the follow_symlinks parameter [1]. When follow_symlinks is set to False, and the source path is a symbolic link, the function will operate on the symbolic link itself rather than the file it points to [1]. Support for this parameter varies by platform [1]. You can verify if follow_symlinks is supported for os.link on your specific platform by checking the os.supports_follow_symlinks set, which contains the functions that support this argument [1]. If a platform does not support follow_symlinks for a particular function, attempting to use it will raise a NotImplementedError [1]. The follow_symlinks parameter was added to os.link in Python 3.3 [1].
Citations:
🌐 Web query:
site:github.com/python/cpython posix_link follow_symlinks AT_SYMLINK_FOLLOW linkat
💡 Result:
In POSIX systems and the C standard library, the linkat system call provides a way to create hard links with behavior that can be controlled via the flags argument [1][2]. The AT_SYMLINK_FOLLOW flag is used specifically with linkat to determine how to handle symbolic links when the source path points to one [1][2]: - When AT_SYMLINK_FOLLOW is included in the flags argument, linkat will dereference the symbolic link and create a hard link to the file the symlink points to [1]. - When the AT_SYMLINK_FOLLOW flag is absent, linkat does not dereference the symbolic link; instead, it creates a new hard link to the symbolic link itself [1][2]. This functionality is significant because, as defined by POSIX, the behavior of the standard link function when encountering a symbolic link is implementation-defined [1][3]. Consequently, different operating systems may handle symbolic links differently by default [1][2][3]. For instance, on some systems, link may follow the symlink, while on others, it does not [1][2][3]. Using linkat with explicit flags is considered the standard, portable way to ensure consistent behavior across different platforms [1][2]. Python's os.link function has evolved to utilize linkat whenever available to provide consistent control over this behavior, addressing historical inconsistencies between platforms [4][5][6]. Users requiring precise control over whether to follow symlinks when creating hard links should rely on the linkat interface or the corresponding Python os.link parameters where available [4][5][7].
Citations:
- 1: os.link(..., follow_symlinks=False) without linkat(3) python/cpython#85527
- 2: os.link(..., follow_symlinks=True) broken on Linux python/cpython#81793
- 3: gh-81793: always call linkat() from os.link(), if available python/cpython#24997
- 4: gh-81793: Always call linkat() from os.link(), if available python/cpython#132517
- 5: Clarify os.link behavior with follow_symlinks=True on POSIX systems python/cpython#138623
- 6: Wrong
follow_symlinksdefault fallback implementation python/cpython#119885 - 7: Add os.linkat() function to create a hard link with flags python/cpython#136413
🏁 Script executed:
#!/bin/bash
set -eu
for version in 3.11 3.12 3.13 3.14; do
url="https://github.com/@raw/python/cpython/v${version}.0/Modules/posixmodule.c"
printf '\n--- CPython %s posix_link implementation ---\n' "$version"
curl -fsSL "$url" |
grep -n -A115 -B15 'os_link_impl\|posix_link\|linkat' |
grep -E -m 140 -n 'os_link_impl|posix_link|HAVE_LINKAT|AT_SYMLINK_FOLLOW|follow_symlinks|linkat|link\('
done
printf '\n--- CPython issue/PR metadata ---\n'
for endpoint in \
https://github.com/@api/repos/python/cpython/issues/81793 \
https://github.com/@api/repos/python/cpython/issues/119885 \
https://github.com/@api/repos/python/cpython/pulls/132517; do
printf '\n### %s\n' "$endpoint"
curl -fsSL "$endpoint" | jq '{title,state,merged_at,html_url,body}'
doneRepository: zhsks311/didimlog
Length of output: 18289
🏁 Script executed:
#!/bin/bash
set -eu
for version in 3.11 3.12 3.13 3.14; do
url="https://github.com/@raw/python/cpython/v${version}.0/Modules/posixmodule.c"
printf '\n--- CPython %s complete os.link branch ---\n' "$version"
curl -fsSL "$url" | sed -n '/os_link_impl(PyObject/,/PyDoc_STRVAR(posix_link__doc__/p' | sed -n '1,155p'
done
printf '\n--- CPython build checks for linkat ---\n'
for branch in 3.11 3.12 3.13 3.14; do
printf '\n### branch %s configure references\n' "$branch"
curl -fsSL "https://github.com/@raw/python/cpython/${branch}/configure.ac" |
grep -n -C 4 'linkat'
doneRepository: zhsks311/didimlog
Length of output: 25650
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import os
import platform
import sys
import tempfile
print("python:", sys.version)
print("platform:", platform.platform())
print("os.link supports follow_symlinks:", os.link in os.supports_follow_symlinks)
print("supports_dir_fd:", os.link in os.supports_dir_fd)
with tempfile.TemporaryDirectory() as root:
parent = os.open(root, os.O_RDONLY | getattr(os, "O_DIRECTORY", 0))
try:
with open(os.path.join(root, "source"), "wb") as handle:
handle.write(b"probe")
try:
os.link(
"source",
"target",
src_dir_fd=parent,
dst_dir_fd=parent,
follow_symlinks=False,
)
except BaseException as exc:
print("exact-call:", type(exc).__name__, str(exc))
else:
print("exact-call: success")
finally:
os.close(parent)
PY
printf '\n--- repository support wording ---\n'
rg -n -i -C 3 'macos|mac os|linux|platform|supported|python 3' README.md CONTRIBUTING.md SECURITY.md pyproject.toml .github/workflowsRepository: zhsks311/didimlog
Length of output: 1748
🌐 Web query:
Apple macOS linkat availability version linkat man page
💡 Result:
The linkat system call is available on macOS and has been supported since macOS 10.10 (OS X Yosemite) [1][2]. It is documented in the link(2) manual page [1][3], which covers both the traditional link and the linkat system call [1]. The function signature for linkat is defined as: int linkat(int fd1, const char *name1, int fd2, const char *name2, int flag); This system call allows for creating hard links relative to directory file descriptors, providing functionality equivalent to link with added flexibility for path resolution [1][3]. Key details regarding its availability and usage on macOS include: Availability: Introduced in macOS 10.10 [2]. Developers requiring compatibility with older versions of macOS (e.g., 10.9 and earlier) often use weak linking or feature detection to avoid runtime errors, as the symbol will be missing on those legacy systems [4]. Manual Page: The documentation is integrated into the link(2) manual page [1][3]. You can access it by running man 2 link in the macOS terminal, which typically displays information for both functions. Behavior: Like its counterparts on other POSIX-compliant systems, linkat does not follow symbolic links by default unless the AT_SYMLINK_FOLLOW flag is specified [1][3][5]. If passed the special value AT_FDCWD for the file descriptor arguments, it behaves identically to the standard link system call [1][3]. Top results: [1], [3], [2]
Citations:
- 1: https://www.manpagez.com/man/2/linkat/osx-10.12.3.php
- 2: x/sys/unix: add *at (POSIX.1-2008 conformed) functions on darwin. golang/go#14458
- 3: https://man.freebsd.org/cgi/man.cgi?manpath=macOS+13.6.5&query=linkat&sektion=2
- 4: jaisnan/rust-dev@a5fe19d
- 5: https://keith.github.io/xcode-man-pages/symlink.7.html
🌐 Web query:
CPython os.link os.supports_follow_symlinks macOS linkat runtime NotImplementedError
💡 Result:
In recent versions of CPython, particularly following the implementation of gh-81793, the behavior of os.link(..., follow_symlinks=False) on macOS has been clarified to ensure it consistently honors the follow_symlinks parameter by utilizing the linkat system call [1][2]. On macOS, the linkat system function is available as of macOS 10.10 [3]. CPython handles this by using a runtime check for linkat. If you attempt to call os.link(..., follow_symlinks=False) on a system where linkat is not available at runtime (such as an older macOS version or specific restricted environments), CPython will raise an error [1][2]. Historically, os.link behaviors were inconsistent across platforms because standard link calls could have different symlink-following behaviors defined by the OS [4][5][6]. While older documentation or legacy versions might have suggested or exhibited a NotImplementedError when using the follow_symlinks parameter if the platform lacked support [7], the modern approach in CPython is to rely on linkat whenever possible [1][2]. Consequently, if your environment explicitly lacks linkat, os.link will not support the follow_symlinks parameter and will raise an error when it is used [1][2].
Citations:
- 1: gh-81793: Always call linkat() from os.link(), if available python/cpython#132517
- 2: python/cpython@5a57248
- 3: https://github.com/python/cpython/blob/722229e5dc1e499664966e50bb98065670033300/Modules/posixmodule.c
- 4: os.link(..., follow_symlinks=False) without linkat(3) python/cpython#85527
- 5: Wrong
follow_symlinksdefault fallback implementation python/cpython#119885 - 6: https://mail.python.org/pipermail/python-bugs-list/2020-July/450942.html
- 7: https://hg.python.org/cpython/rev/27f9c26fdd8b
🌐 Web query:
Python 3.11 3.12 3.13 3.14 macOS minimum supported version official docs
💡 Result:
The official Python documentation states that installers provided by python.org are generally designed to run on a wide range of macOS versions [1][2][3]. For modern Python releases (3.11 through 3.14), the minimum supported macOS version has evolved from 10.9 (Mavericks) to 10.13 (High Sierra) [2][4][5][6]. Key details regarding macOS support: - Python 3.11 and 3.12: Historically supported starting from macOS 10.9 [6]. - Python 3.13 and 3.14: The Python release team officially bumped the minimum supported macOS version to 10.13 (High Sierra) for these releases [2][4][5]. This change was implemented to align with Apple's support lifecycle for Xcode and Command Line Tools, as well as to facilitate the building and testing of newer features such as free-threading [4][5]. For the most accurate requirement for a specific sub-version or installer build, you should consult the Read Me file included with the specific python.org installer package [1][2][3]. By default, this file is installed in the /Applications/Python [Version]/ directory [1][2][3]. Users on macOS versions older than 10.13 who wish to use newer Python releases may need to use alternative distributors, such as the MacPorts project, which often provides support for a wider range of macOS versions [4][5]. If you need to run Python on a very old macOS version, building Python from source is also an option, which can often be configured for older systems [4][5][6].
Citations:
- 1: https://docs.pythonlang.net/3/using/mac.html
- 2: https://github.com/python/cpython/blob/208b0fb645c0e14b0826c0014e74a0b70c58c9d6/Doc/using/mac.rst
- 3: https://docs.python.org/3/using/mac.html
- 4: https://discuss.python.org/t/macos-version-support-policy/53715
- 5: https://discuss.python.org/t/macos-version-support-policy/53715/1
- 6: https://github.com/python/cpython/blob/main/Mac/BuildScript/README.rst
🌐 Web query:
site:docs.python.org/3.11/using/mac.html macOS 10.9 minimum Python 3.11
💡 Result:
Python 3.11 does not support macOS 10.9 (Mavericks) [1]. Official Python 3.11 distributions and modern Python installers for macOS typically require significantly newer versions of the operating system than 10.9 [1]. macOS 10.9 is legacy software that lacks the necessary system frameworks, security updates, and architecture support required to run contemporary Python 3.11 binaries [1]. Users attempting to run Python 3.11 on such an outdated operating system would be unable to use official pre-compiled installers [1].
Citations:
🌐 Web query:
site:docs.python.org/3.14/using/mac.html macOS minimum supported version
💡 Result:
Python 3.14’s macOS installer currently supports macOS 10.15 Catalina and later. (docs.python.org)
Citations:
🏁 Script executed:
#!/bin/bash
set -eu
for version in 3.11 3.12 3.13 3.14; do
printf '\n--- Python %s macOS documentation ---\n' "$version"
curl -fsSL "https://docs.python.org/${version}/using/mac.html" |
sed 's/<[^>]*>/ /g' |
tr -s '[:space:]' ' ' |
grep -o -i -E '.{0,180}(macOS|OS X|minimum|10\.[0-9]+).{0,260}' |
head -n 12
doneRepository: zhsks311/didimlog
Length of output: 17427
Declare macOS 10.10 as the minimum or provide a fallback. The src_dir_fd and dst_dir_fd arguments require linkat; follow_symlinks=False does not require AT_SYMLINK_FOLLOW. On older macOS versions, CPython raises NotImplementedError, so file creation cannot publish the target.
🤖 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 `@src/didimlog/conditional_file.py` around lines 214 - 220, Update the os.link
call in the conditional file publication logic to support macOS versions before
10.10: either declare macOS 10.10 as the minimum supported version or provide a
fallback when src_dir_fd/dst_dir_fd raise NotImplementedError. Preserve atomic
target publication and the existing follow_symlinks=False behavior.
| def _planned_knowledge_is_ignored( | ||
| project_root: Path, | ||
| exclude_path: Path, | ||
| intended: bytes | None, | ||
| ) -> bool: | ||
| excludes_file = _read_optional_config_path(project_root) | ||
| ignore_case = _read_ignore_case(project_root) | ||
| try: | ||
| with tempfile.TemporaryDirectory( | ||
| prefix="didimlog-git-exclude-" | ||
| ) as temporary: | ||
| git_directory = Path(temporary) / "git" | ||
| (git_directory / "info").mkdir(parents=True) | ||
| (git_directory / "objects").mkdir() | ||
| (git_directory / "refs" / "heads").mkdir(parents=True) | ||
| (git_directory / "HEAD").write_bytes(b"ref: refs/heads/main\n") | ||
| (git_directory / "config").write_bytes( | ||
| b"[core]\n\trepositoryformatversion = 0\n\tbare = false\n" | ||
| ) | ||
| if intended is not None: | ||
| (git_directory / "info" / "exclude").write_bytes(intended) | ||
|
|
||
| arguments = [ | ||
| f"--git-dir={git_directory}", | ||
| f"--work-tree={project_root}", | ||
| "-c", | ||
| f"core.ignoreCase={'true' if ignore_case else 'false'}", | ||
| ] | ||
| configured_exclude = ( | ||
| Path(excludes_file) | ||
| if excludes_file is not None and Path(excludes_file).is_absolute() | ||
| else project_root / excludes_file | ||
| if excludes_file is not None | ||
| else None | ||
| ) | ||
| if configured_exclude is not None: | ||
| if _same_file(configured_exclude, exclude_path): | ||
| effective_exclude = Path(temporary) / "empty-excludes" | ||
| effective_exclude.write_bytes(b"") | ||
| else: | ||
| effective_exclude = Path(excludes_file) | ||
| arguments.extend(("-c", f"core.excludesFile={effective_exclude}")) | ||
| arguments.extend( | ||
| ("check-ignore", "--no-index", "-q", "--", "knowledge/") | ||
| ) | ||
| result = _run_git( | ||
| project_root, | ||
| tuple(arguments), | ||
| allowed_returncodes=(0, 1), | ||
| ) | ||
| return result.returncode == 0 | ||
| except OSError as error: | ||
| raise _git_unavailable() from error |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
git check-ignore --no-index option introduced version
💡 Result:
The --no-index option for git check-ignore was introduced in Git version 1.8.5, which was released in December 2013 [1][2]. This feature was added via commit 8231fa6, which allows the command to bypass the index when checking files [3]. By default, git check-ignore does not produce output for tracked files because they are not subject to ignore rules; the --no-index option enables users to debug why a specific path became tracked by allowing those paths to be checked against ignore patterns [3][4].
Citations:
- 1: https://stackoverflow.com/questions/12144633/explain-which-gitignore-rule-is-ignoring-my-file
- 2: https://lwn.net/Articles/575726/
- 3: git/git@8231fa6
- 4: https://git-scm.com/docs/git-check-ignore
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Git-version requirements and related checks ---'
rg -n -i --glob '!*.lock' --glob '!*.sum' \
'git( version)?|minimum.*git|git.*(version|required|support)|check-ignore|PROJECT_EXCLUDE_GIT_UNAVAILABLE' \
README* pyproject.toml setup.cfg setup.py .github src tests 2>/dev/null || true
printf '%s\n' '--- Relevant helper and call sites ---'
rg -n -A8 -B8 \
'_planned_knowledge_is_ignored|def _run_git|PROJECT_EXCLUDE_GIT_UNAVAILABLE|git_unavailable' \
src tests 2>/dev/null || trueRepository: zhsks311/didimlog
Length of output: 50374
🌐 Web query:
site:git-scm.com/docs/git-check-ignore --no-index Git 1.8.5 release
💡 Result:
The --no-index option is a command-line flag used with specific Git commands, most notably git check-ignore [1][2]. It instructs Git to perform operations without referencing the index [1][3]. Regarding Git 1.8.5, there is no evidence that the --no-index option was introduced in that specific version, nor is there information confirming 1.8.5 as a major release associated with this feature. The --no-index flag is documented as a mechanism to debug why paths are or are not being ignored by Git rules, allowing users to check exclusion patterns against files regardless of their current status in the index [1][4]. Official documentation indicates that git check-ignore has maintained this functionality across various versions [1][5][6][7].
Citations:
- 1: https://git-scm.com/docs/git-check-ignore
- 2: https://git-scm.com/docs/git-check-ignore/2.8.6
- 3: https://git-scm.com/docs/git-check-ignore/ru
- 4: https://git-scm.com/docs/git-check-ignore/ja.html
- 5: https://git-scm.com/docs/git-check-ignore/2.0.5
- 6: http://www.git-scm.com/docs/git-check-ignore/2.36.0
- 7: https://git-scm.com/docs/git-check-ignore/2.18.0
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate project metadata ---'
git ls-files | grep -E '(^|/)(README([^/]*)?|pyproject\.toml|setup\.cfg|setup\.py|tox\.ini|\.github/|docs?/)' | head -200
printf '%s\n' '--- targeted version references ---'
for f in README.md README.rst README.txt pyproject.toml setup.cfg setup.py tox.ini; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
rg -n -i 'git|version|require' "$f" || true
fi
done
printf '%s\n' '--- targeted source references ---'
rg -n -A12 -B12 \
'_planned_knowledge_is_ignored|def _run_git|PROJECT_EXCLUDE_GIT_UNAVAILABLE|_git_unavailable' \
src/didimlog tests 2>/dev/null | head -500
printf '%s\n' '--- CI references ---'
rg -n -i -A3 -B3 'git|ubuntu|macos|windows' .github 2>/dev/null | head -300Repository: zhsks311/didimlog
Length of output: 41312
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import urllib.request
urls = [
"https://github.com/@raw/git/git/master/Documentation/git-check-ignore.adoc",
"https://github.com/@raw/git/git/v2.0.5/Documentation/git-check-ignore.txt",
"https://github.com/@raw/git/git/v1.8.4/Documentation/git-check-ignore.txt",
"https://github.com/@raw/git/git/v1.8.5/Documentation/git-check-ignore.txt",
]
for url in urls:
print(f"\n--- {url} ---")
try:
text = urllib.request.urlopen(url, timeout=10).read().decode()
except Exception as exc:
print(type(exc).__name__, exc)
continue
lines = text.splitlines()
for i, line in enumerate(lines):
if "--no-index" in line or "check-ignore" in line:
print("\n".join(f"{j+1}: {lines[j]}" for j in range(max(0,i-2), min(len(lines),i+3))))
PYRepository: zhsks311/didimlog
Length of output: 1104
🏁 Script executed:
#!/bin/bash
set -eu
fetch() {
url="$1"
printf '\n--- %s ---\n' "$url"
curl -k -L --fail --silent "$url"
}
fetch 'https://github.com/@raw/git/git/v1.8.4/Documentation/git-check-ignore.txt' | grep -n -A4 -B4 -- '--no-index' || true
fetch 'https://github.com/@raw/git/git/v1.8.5/Documentation/git-check-ignore.txt' | grep -n -A4 -B4 -- '--no-index' || true
printf '\n--- introducing commit ---\n'
curl -k -L --fail --silent \
'https://github.com/@api/repos/git/git/commits/8231fa6ae1ef92be73bec5cf0a533a4d8b71d536' |
jq '{sha:.sha,date:.commit.author.date,message:.commit.message,parents:[.parents[].sha]}'
printf '\n--- introducing commit files ---\n'
curl -k -L --fail --silent \
'https://github.com/@api/repos/git/git/commits/8231fa6ae1ef92be73bec5cf0a533a4d8b71d536' |
jq -r '.files[] | .filename'Repository: zhsks311/didimlog
Length of output: 671
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Git 1.8.4 and 1.8.5 release dates ---'
for tag in v1.8.4 v1.8.5; do
curl -k -L --fail --silent \
"https://github.com/@api/repos/git/git/git/ref/tags/${tag}" |
jq -r '"\(.ref): type=\(.object.type) sha=\(.object.sha)"'
done
printf '%s\n' '--- project documentation and packaging excerpts ---'
sed -n '1,70p' README.md
sed -n '1,45p' pyproject.tomlRepository: zhsks311/didimlog
Length of output: 3258
🏁 Script executed:
#!/bin/bash
set -u
for version in 1.8.4 1.8.5; do
url="https://github.com/@raw/git/git/v${version}/Documentation/git-check-ignore.txt"
printf '\n--- Git %s ---\n' "$version"
body="$(mktemp)"
status="$(curl -k -L --silent --show-error --output "$body" --write-out '%{http_code}' "$url" 2>&1)"
printf 'HTTP/status: %s\n' "$status"
if [ "$status" = 200 ]; then
if grep -n -A5 -B3 -- '--no-index' "$body"; then
printf 'option: present\n'
else
printf 'option: absent\n'
fi
else
head -c 300 "$body"
printf '\n'
fi
rm -f "$body"
done
printf '\n--- Git repository tag refs ---\n'
git ls-remote --tags https://github.com/git/git.git \
'refs/tags/v1.8.4' 'refs/tags/v1.8.5' 2>&1 || trueRepository: zhsks311/didimlog
Length of output: 826
Document Git 1.8.5 or newer as a requirement
git check-ignore --no-index is absent in Git 1.8.4. Older Git causes didim setup to fail with PROJECT_EXCLUDE_GIT_UNAVAILABLE.
🤖 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 `@src/didimlog/project/git_exclude.py` around lines 427 - 479, Document Git
1.8.5 or newer as a prerequisite for the _planned_knowledge_is_ignored flow,
because it invokes check-ignore with --no-index. Update the project’s
appropriate requirements or setup documentation, explicitly noting that older
Git versions are unsupported and can cause didim setup to report
PROJECT_EXCLUDE_GIT_UNAVAILABLE.
|
최종 리뷰에서 확인한 비차단 후속 작업은 #4에 backlog로 정리했습니다. v0.0.1 배포와 분리해 추적합니다. |
변경 내용
검증
uv run --project . python -m unittest discover -s tests -v: 372 tests passeddidim --version,didim --help성공승격 조건
v0.0.1release/tag 및 PyPI publish 진행Summary by CodeRabbit
didimcommand-line tool for setup, lesson and record creation, indexing, status, diagnostics, and Claude integration.