Skip to content

fix(analysis): allow safe contained instruction aliases - #51

Merged
phodal merged 6 commits into
QoderAI:mainfrom
oldwinter:codex/contained-structural-symlinks
Aug 3, 2026
Merged

fix(analysis): allow safe contained instruction aliases#51
phodal merged 6 commits into
QoderAI:mainfrom
oldwinter:codex/contained-structural-symlinks

Conversation

@oldwinter

Copy link
Copy Markdown
Contributor

Summary

  • allow direct, relative, same-directory tracked instruction-route symlinks to resolve inside a Git workspace
  • reject links that escape the root, target ignored/untracked/non-structural files, alias manifests, cross ownership scopes, or introduce symlink chains
  • use the canonical target for reads and document the accepted topology contract

Verification

  • node --test test/workspace-topology.test.mjs (16 tests passed)
  • npm run check after npm ci (1101 tests passed; npm package and runtime bundle verification passed)

No matching open upstream issue was found.

oldwinter and others added 5 commits August 2, 2026 04:59
Resolve structural entries canonically so tracked provider adapters inside the topology root remain inspectable while escaping links stay rejected.

Tests: node --test test/workspace-topology.test.mjs; frozen general-tasks evidence-bundle

Co-authored-by: Codex (GPT 5.6 Sol) <codex@openai.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the workspace-topology discovery logic to allow a narrowly-scoped class of safe, contained, tracked instruction-route symlinks (e.g., CLAUDE.md -> AGENTS.md) while continuing to reject unsafe symlink patterns. It also documents the topology contract and adds regression tests to lock in the intended behavior.

Changes:

  • Allow direct, same-directory, tracked instruction-route symlink redirects inside the Git root and read content via the canonical target path.
  • Add workspace topology tests covering accepted contained instruction links and multiple rejected symlink topologies (cross-scope, chained hop, ignored/untracked/non-structural targets, manifest aliasing).
  • Add a spec describing intent, acceptance scenarios, non-goals, and verification evidence.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
test/workspace-topology.test.mjs Adds regression coverage for accepting a contained tracked instruction symlink and rejecting unsafe symlink variants.
scripts/workspace-topology/manifests.mjs Implements canonical-path-based structural safety checks and narrowly permits safe instruction-route symlink redirects.
docs/specs/2026-08-02-contained-structural-symlinks.md Documents the accepted symlink topology contract, risks, and verification steps.
Suppressed comments (1)

scripts/workspace-topology/manifests.mjs:192

  • safeRedirect relies on case-sensitive route comparisons (canonicalRoute === item.route, trackedRoutes.has(canonicalRoute), and dirnameRoute(...) === ...). On Windows’ default case-insensitive filesystems this can incorrectly mark safe structural files/links as unsafe when realpath() returns different casing. Suggest using a win32-normalized key (e.g. lowercased) for these route comparisons while keeping the original route values for reporting.
      const canonicalRoute = path.relative(canonicalRoot, canonical).split(path.sep).join("/");
      const safeRedirect = canonicalRoute === item.route
        || (
          item.provenance === "tracked"
          && trackedRoutes.has(canonicalRoute)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +171 to +174
const target = path.normalize(await readlink(absolute));
return !path.isAbsolute(target)
&& path.dirname(target) === "."
&& path.resolve(path.dirname(absolute), target) === canonical;
Compare structural routes and link targets through a shared pathIdentityKey
helper that folds case only on Windows. Native realpath on Windows reports
on-disk casing, so a case-only divergence from the tracked inventory route
would otherwise mark an ordinary file structure-entry-unsafe and fail a
normal-depth Evidence Bundle.

Validate the redirect target against the canonical directory instead of the
caller-supplied root, so the accept decision no longer depends on the caller
passing an already-realpath'd topology root.

Drop the unused canonical read path: manifest aliases are rejected, so
item.canonical always equalled the route-based path, and requiring it in
readInventoryFile turned a future contract break into a silently swallowed
manifest-read-unavailable warning.

Lock the surrounding contract with regressions for a nested-package alias, an
absolute in-root target, a dangling link that now reports
structure-entry-unavailable, and platform-scoped route identity. Record the
cross-directory adapter limitation explicitly in the spec Non-goals, since
those routes still fail a normal Evidence Bundle.

Tests: npm run check (1105 tests passed; pack verification passed)

Co-authored-by: QoderAI <qoder_ai@qoder.com>
@phodal

phodal commented Aug 3, 2026

Copy link
Copy Markdown
Member

Reviewed and pushed one follow-up commit to this branch (09d139f) rather than blocking on review comments. Thanks for the tight safety model — every rejection path fails closed, which made the remaining issues easy to isolate.

What the follow-up changes:

  1. Windows case identity (behavior risk). canonicalRoute === item.route now gates every structural entry, not just links. Native realpath on Windows returns on-disk casing, so a case-only divergence from the tracked inventory route would mark an ordinary file structure-entry-unsafe and fail a normal-depth Evidence Bundle. Route and target comparisons now go through a shared pathIdentityKey helper in contract.mjs that folds case only on win32. The redirect branch also resolves the tracked target's original route before calling isInstructionRoute, so a realpath casing difference cannot silently disqualify a valid adapter.
  2. Dropped the canonical read path. Since manifest aliases are rejected, item.canonical always equalled the route-based path. Reverting readInventoryFile to route-based reads is behavior-neutral (verified: the topology suite still passes with the change reverted in isolation) and removes an implicit item.canonical requirement whose failure mode was a silently swallowed manifest-read-unavailable.
  3. Removed an implicit precondition. isDirectSameDirectoryLink compared against path.dirname(absolute), which is derived from the caller-supplied root. That holds today only because index.mjs realpaths the workspace first; any caller passing a non-canonical root would have had every valid alias silently rejected. It now compares against path.dirname(canonical), which is equivalent given the same-directory check.
  4. Regressions added: nested-package alias (worked but was untested), absolute in-root target rejection, and the dangling-link warning code — that one changed from structure-entry-unsafe on main to structure-entry-unavailable here, which is more honest but was undocumented, so it is now an AC.
  5. Spec Non-goals now state explicitly that cross-directory adapter links and absolute targets keep failing a normal Evidence Bundle, and the spec cross-references the monorepo topology contract.

Verification: npm run check — 1105 tests passed, pack verification passed.

One scope note, filed as #53 rather than expanded here: the same-directory restriction means the most common adapter convention (.github/copilot-instructions.md, .claude/CLAUDE.md, or .cursor/rules/*.mdc linking up to the root guide) still yields partial topology, and evidence-bundle/index.mjs turns that into a failed bundle at normal depth. So the motivating class of repositories is only partly unblocked. Since the target must already be tracked, in-root, and a single hop, allowing .. looks safe, but that is a deliberate design call for a separate change.

@phodal
phodal merged commit bbed86e into QoderAI:main Aug 3, 2026
4 checks passed
creayma-del pushed a commit to creayma-del/better-harness that referenced this pull request Aug 3, 2026
…ess-gate

Integrate main (PRs QoderAI#51-QoderAI#55) into the loop-readiness feature branch.
Resolved conflict in test/scripts-refactor-contract.test.mjs:
- Recalculated sha256 baselines for `commands --json` and `schema` to
  reflect the merged command registry (Grok host adapter added by PR QoderAI#52).

All 1120 tests pass after merge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants